Draft
Conversation
shubh-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
WalkthroughAdds a beta-gated Mock Server feature: new Electron Express mock-server + IPC handlers, renderer Redux slice and thunks, dashboard UI (RouteTable, RequestLog), FilterDropdown, preferences beta wiring, tab/statusbar/sidebar integrations, tests and fixtures, and related styling components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer as Renderer Process
participant Main as Main Process
participant Mock as MockServer
participant FS as File System
participant Client as External Client
rect rgba(0,150,255,0.5)
User->>Renderer: Click "Start Mock Server" (dashboard)
Renderer->>Renderer: dispatch startMockServer thunk
Renderer->>Main: ipc.invoke('renderer:start-mock-server', { collectionUid, port })
Main->>FS: Read collection example files
FS-->>Main: Return files
Main->>Mock: start(collectionUid, collectionPath, port)
Mock->>Mock: Build routeMap, count routes/examples
Mock-->>Main: { routeCount, exampleCount }
Main->>Renderer: ipc.send('main:mock-server-status-changed', status)
Renderer->>Renderer: update Redux (servers/routes)
end
rect rgba(0,200,100,0.5)
Client->>Mock: HTTP GET /path (with optional X-Mock-Example / X-Mock-Response-Code)
Mock->>Mock: Normalize path, lookup routeMap
Mock->>Mock: Select example (header match or default)
Mock->>Mock: Apply global delay, set headers, log request
Mock-->>Client: Response (status, headers, body)
Mock->>Main: emit 'main:mock-server-request-log' with entry
Main->>Renderer: forward IPC event
Renderer->>Renderer: addRequestLogEntry -> UI updates
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (13)
packages/bruno-electron/src/app/mock-server.js (1)
328-336: Consider adding a timeout forhttpServer.close().If connections remain open,
close()can hang indefinitely. A timeout orserver.closeAllConnections()(Node 18.2+) would preventstop()from blocking forever during shutdown.♻️ Proposed fix with timeout
const stop = async (collectionUid) => { const server = servers.get(collectionUid); if (!server) return; - await new Promise((resolve) => server.httpServer.close(resolve)); + await new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + server.httpServer.closeAllConnections?.(); + resolve(); + }, 5000); + server.httpServer.close((err) => { + clearTimeout(timeout); + resolve(); + }); + }); servers.delete(collectionUid); emitStatusChanged(collectionUid, { status: 'stopped', port: null, routeCount: 0, exampleCount: 0, globalDelay: 0 }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/app/mock-server.js` around lines 328 - 336, The stop function may hang because server.httpServer.close() can wait indefinitely if connections remain open; update the stop(collectionUid) logic to race the close call against a timeout and/or call server.httpServer.closeAllConnections() when available: call server.httpServer.close(...) but wrap it in a Promise that rejects or resolves after a short timeout (e.g., a few seconds), attempt server.httpServer.closeAllConnections() before or when the timeout triggers on Node >=18.2, ensure servers.delete(collectionUid) and emitStatusChanged(...) still run after the close/timeout so shutdown proceeds reliably.packages/bruno-electron/package.json (1)
60-61: Minor:expressplacement breaks alphabetical order.
expressappears betweenelectron-storeandelectron-util. Consider moving it afterelectron-utilfor consistency, though this is purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/package.json` around lines 60 - 61, In package.json's dependencies, the "express" entry is out of alphabetical order between "electron-store" and "electron-util"; reorder the dependencies so "express" appears after "electron-util" to restore consistent alphabetical ordering of dependency keys.packages/bruno-app/src/components/FilterDropdown/StyledWrapper.js (1)
23-24: Use a theme fallback instead of#666inrgba(...).Prefer a theme-derived fallback color so this component remains fully theme-managed.
As per coding guidelines, "Use styled component's theme prop to manage CSS colors and not CSS variables when in the context of a styled component or any React component using styled components."
Also applies to: 87-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/FilterDropdown/StyledWrapper.js` around lines 23 - 24, The styled wrapper uses a hardcoded fallback '#666' inside rgba(...) instead of a theme-derived default; update both occurrences where rgba(props.theme.dropdown.selectedColor || '#666', 0.5) is used to reference a fallback from the theme (e.g., props.theme.colors?.neutral600 or props.theme.dropdown?.defaultColor) so the component remains fully theme-managed; locate the expressions in StyledWrapper.js (the styled component using props.theme.dropdown.selectedColor) and replace the literal '#666' with the chosen theme fallback key, ensuring the optional chaining matches existing theme shape and preserves the 0.5 alpha.packages/bruno-app/src/components/MockServerDashboard/RequestLog/StyledWrapper.js (1)
52-67: Move method/status badge colors to theme tokens.Using hardcoded badge colors here will drift from global theme behavior and complicate future theme updates.
As per coding guidelines, "Use styled component's theme prop to manage CSS colors and not CSS variables when in the context of a styled component or any React component using styled components."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/MockServerDashboard/RequestLog/StyledWrapper.js` around lines 52 - 67, The badge colors for HTTP methods (selectors like &.get, &.post, &.put, &.patch, &.delete) and status codes (selectors under .status-code: &.status-2xx, &.status-3xx, &.status-4xx, &.status-5xx, &.status-unmatched) are hardcoded; update these to use styled-components theme tokens (via props.theme) instead of hex values so colors follow the global theme. Replace each hex with the appropriate theme lookup (e.g., theme.colors.method.get, theme.colors.status.2xx or your app's badge color tokens), provide sensible fallbacks if needed, and ensure the selectors in StyledWrapper.js reference theme values consistently for both method and status badges.packages/bruno-app/src/components/MockServerDashboard/StyledWrapper.js (1)
24-38: Replace hardcoded/CSS-variable colors with theme tokens in this styled component.This file mixes theme colors with
#hexandvar(--...)values, which makes theming inconsistent and harder to maintain.As per coding guidelines, "Use styled component's theme prop to manage CSS colors and not CSS variables when in the context of a styled component or any React component using styled components."
Also applies to: 62-63, 96-97, 115-124, 133-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/MockServerDashboard/StyledWrapper.js` around lines 24 - 38, The styled component StyledWrapper uses hardcoded hex and CSS variable colors for states (.running, .starting, .error) and elsewhere; replace those literal values with theme tokens accessed via the styled-components theme prop (e.g., props.theme.colors.status.running / .starting / .error or the appropriate theme keys used across the app) and update all other hardcoded occurrences (the locations noted around the later blocks) to use theme.colors.* instead of `#hex` or var(--...). Keep the same class selectors (.running, .starting, .error) and animation, only swap the color values to the theme tokens to ensure consistent theming.packages/bruno-app/src/components/MockServerDashboard/RouteTable/StyledWrapper.js (1)
37-38: Keep route-table color styling theme-driven.The focus border and method badge colors should come from
props.themeinstead ofvar(--...)and hardcoded hex values to stay aligned with app theming.As per coding guidelines, "Use styled component's theme prop to manage CSS colors and not CSS variables when in the context of a styled component or any React component using styled components."
Also applies to: 47-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/MockServerDashboard/RouteTable/StyledWrapper.js` around lines 37 - 38, Replace hardcoded CSS variables and hex colors in the styled components with theme values from props.theme: update the border-color in StyledWrapper (focus state) to use something like props.theme['tabActiveBorder'] (or the correct key from the app theme), and similarly change the method badge's background and text color definitions (in the MethodBadge/Badge styled component) to use props.theme keys instead of hex literals or var(--...) so all route-table colors are driven by props.theme; ensure you reference and use the exact theme property names used elsewhere in the app and adjust the styled-components selectors in StyledWrapper.js accordingly.packages/bruno-app/src/components/MockServerDashboard/RequestLog/index.js (2)
17-25: Hardcoded 'en-US' locale may not match user preference.
toLocaleTimeString('en-US', ...)forces US format. Usingundefinedas the locale lets the browser use the user's system locale.Proposed fix
- return d.toLocaleTimeString('en-US', { hour12: false, hour: '2-digit', minute: '2-digit', second: '2-digit' }) + return d.toLocaleTimeString(undefined, { hour12: false, hour: '2-digit', minute: '2-digit', second: '2-digit' })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/MockServerDashboard/RequestLog/index.js` around lines 17 - 25, The formatTimestamp function forces a US locale by calling toLocaleTimeString('en-US', ...); update formatTimestamp to use the user's system locale by passing undefined (or omitting the locale argument) instead of the hardcoded 'en-US' so the browser uses the runtime locale; ensure the rest of the options and millisecond padding logic in formatTimestamp remain unchanged (function name: formatTimestamp).
63-68: Auto-scroll may disrupt user who scrolled up to inspect older entries.The effect scrolls to bottom whenever
filteredLogs.lengthchanges. If a user scrolls up to read history and a new entry arrives, they'll lose their position. Consider only auto-scrolling if already near the bottom.Proposed approach
useEffect(() => { - if (tableContainerRef.current) { - tableContainerRef.current.scrollTop = tableContainerRef.current.scrollHeight; + const container = tableContainerRef.current; + if (container) { + const isNearBottom = container.scrollHeight - container.scrollTop - container.clientHeight < 100; + if (isNearBottom) { + container.scrollTop = container.scrollHeight; + } } }, [filteredLogs.length]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/MockServerDashboard/RequestLog/index.js` around lines 63 - 68, The current useEffect tied to filteredLogs.length always forces tableContainerRef to scroll to bottom, which disrupts users who have scrolled up; modify the effect in RequestLog so it only auto-scrolls when the container is already near the bottom (e.g., check tableContainerRef.current.scrollTop + clientHeight >= scrollHeight - THRESHOLD) or on initial mount, and otherwise preserve the user's scroll position; update the useEffect that references filteredLogs.length and tableContainerRef to compute this isNearBottom condition (use a small pixel threshold like 50–150) before setting scrollTop to scrollHeight.packages/bruno-app/src/providers/ReduxStore/slices/mock-server.js (1)
97-103:clearMockLogdoesn't handle IPC failure.Unlike other thunks,
clearMockLogdoesn't checkresult.success. If the IPC handler inmock-server.jsalways returns{ success: true }, this is fine, but if the pattern changes, failures would be silent.Proposed fix for consistency
export const clearMockLog = createAsyncThunk( 'mockServer/clearLog', async ({ collectionUid }) => { - await window.ipcRenderer.invoke('renderer:mock-server-clear-log', { collectionUid }); + const result = await window.ipcRenderer.invoke('renderer:mock-server-clear-log', { collectionUid }); + if (!result.success) { + throw new Error(result.error || 'Failed to clear log'); + } return { collectionUid }; } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/ReduxStore/slices/mock-server.js` around lines 97 - 103, The thunk clearMockLog currently awaits window.ipcRenderer.invoke('renderer:mock-server-clear-log', { collectionUid }) but doesn't inspect the returned result; update clearMockLog to capture the invoke response (e.g., result) and check result.success like the other thunks, throwing or returning a rejected value when success is false so failures are surfaced to Redux and logs; ensure you reference clearMockLog and the 'renderer:mock-server-clear-log' IPC channel when making the change.packages/bruno-app/src/components/MockServerDashboard/index.js (2)
63-69: Consider debouncing delay updates and adding error handling.
handleDelayChangedispatches an IPC call on every keystroke when running. This can flood the main process. Also, errors fromupdateMockDelayare silently swallowed since.unwrap()isn't called.Proposed approach
+import { useMemo } from 'react'; +import debounce from 'lodash/debounce'; // Inside component: + const debouncedUpdateDelay = useMemo( + () => debounce((newDelay) => { + dispatch(updateMockDelay({ collectionUid: collection.uid, delay: newDelay })) + .unwrap() + .catch((err) => toast.error(err.message || 'Failed to update delay')); + }, 300), + [dispatch, collection.uid] + ); const handleDelayChange = (e) => { const newDelay = Number(e.target.value) || 0; setDelay(newDelay); if (isRunning) { - dispatch(updateMockDelay({ collectionUid: collection.uid, delay: newDelay })); + debouncedUpdateDelay(newDelay); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/MockServerDashboard/index.js` around lines 63 - 69, handleDelayChange currently dispatches updateMockDelay on every keystroke and doesn't surface errors; wrap the dispatch in a short debounce (e.g., 300ms) so updates are sent only after typing pauses, and call .unwrap() on the dispatched updateMockDelay promise inside an async handler to catch/rethrow errors (or handle them with try/catch and dispatch/set an error state). Update the handleDelayChange (and any helper like a debounced function) to use the collection.uid and newDelay when invoking dispatch(updateMockDelay(...)). Ensure the debounced function is memoized (e.g., useRef/useCallback) so it isn’t recreated on every render.
121-128: Port input mixes number and string state.Initial
portstate is4000(number), butonChangesets it toe.target.value(string). This works due to JS coercion but can cause subtle bugs. ConsiderNumber(e.target.value)or keep it consistently as a string.Proposed fix
<input type="number" value={isRunning ? serverState.port : port} - onChange={(e) => setPort(e.target.value)} + onChange={(e) => setPort(Number(e.target.value) || 4000)} disabled={isRunning || isStarting} min={1} max={65535} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/MockServerDashboard/index.js` around lines 121 - 128, The port input mixes number and string types: ensure the local port state in the MockServerDashboard component is always a number by initializing port as a number and converting user input to a number in the onChange handler (use Number(e.target.value) or parseInt) so setPort receives a numeric value; update the onChange to call setPort(Number(e.target.value)) and verify any comparisons/uses (e.g., value={isRunning ? serverState.port : port}) assume numeric types.packages/bruno-app/src/components/MockServerDashboard/RouteTable/index.js (1)
31-36: Index-based UID may cause React reconciliation issues if routes change order.Using
route-${idx}as the key means React can't track row identity across re-renders when routes are added/removed mid-list. If routes have a stable identifier (e.g.,method + path), prefer that as the key.Proposed fix
.map((route, idx) => ({ ...route, - uid: `route-${idx}`, + uid: `${route.method}-${route.path}`, hits: hitCounts[`${route.method} ${route.path}`] || 0, source: route.examples?.[0]?.sourceFile || '-' }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/MockServerDashboard/RouteTable/index.js` around lines 31 - 36, The mapping that builds per-row objects currently sets uid to an index-based string (`uid: \`route-${idx}\``) which breaks React reconciliation when order changes; update the mapping in the RouteTable component where you create uid (the block that spreads route and sets uid, hits, source) to compute a stable unique id from the route itself (for example `${route.method} ${route.path}` or another stable combination of route properties) and fall back to a deterministic alternative only if those fields are missing to ensure uniqueness across rows.packages/bruno-app/src/components/FilterDropdown/index.js (1)
75-85: Consider adding keyboard navigation for accessibility.The dropdown options are
divelements withonClickhandlers. They're not focusable or navigable via keyboard (Tab/Arrow keys). For a POC this is acceptable, but production use would benefit fromrole="listbox"/role="option"semantics and keyboard support.Also applies to: 89-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/FilterDropdown/index.js` around lines 75 - 85, The dropdown options (elements with className "filter-option" using onClick and calling handleSelect) are not keyboard-accessible; update the FilterDropdown component to add proper ARIA roles and keyboard handlers: give the container role="listbox" and each option role="option", make each option focusable with tabIndex={0}, and add an onKeyDown handler that calls handleSelect on Enter/Space and moves focus between options on ArrowUp/ArrowDown (maintain/derive an active index similar to isActive); ensure the filter-option-check visibility and any active state logic still reflect keyboard-driven selection.
🤖 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/FilterDropdown/index.js`:
- Around line 65-71: The button element rendered in the FilterDropdown component
(the <button> with className "filter-trigger" that uses selectedLabel,
handleToggle and IconChevronDown) lacks an explicit type and will act as a
submit button when inside a form; update that JSX to include type="button" on
the filter-trigger button to prevent unintended form submissions while
preserving the existing onClick behavior.
In `@packages/bruno-app/src/components/MockServerDashboard/RequestLog/index.js`:
- Around line 166-175: The RequestLog entries use id while EditableTable expects
uid, causing React keys to fallback to indices; fix by mapping log entries
before rendering: in RequestLog where filteredLogs is passed to EditableTable,
transform each log object to include uid (e.g., uid: log.id) or rename id to uid
(preserving other fields), so EditableTable receives rows with row.uid;
alternatively update mock-server.js to emit uid instead of id. Update the data
flow that constructs filteredLogs (used in the EditableTable prop) so every row
has a uid property.
In
`@packages/bruno-app/src/components/ResponseExample/CreateExampleModal/index.js`:
- Around line 123-149: The new select controls for statusCode and bodyType need
data-testid attributes so Playwright can target them; update the <select> with
id="statusCode" (used with value={statusCode} and onChange={setStatusCode}) to
include a data-testid like "status-code-select", and similarly update the
<select> with id="bodyType" (value={bodyType}, onChange={setBodyType}) to
include a data-testid like "body-type-select"; keep the existing
id/value/onChange and STATUS_CODES/BODY_TYPES mappings unchanged—only add the
data-testid attributes to those two select elements.
In `@packages/bruno-app/src/components/StatusBar/index.js`:
- Around line 142-152: The status-bar mock server button in the StatusBar
component lacks a stable test selector; update the <button> rendered in
StatusBar (the one with className "status-bar-button",
onClick={handleMockServerClick} and referencing runningMockServer[1].port) to
include a deterministic data-testid attribute (e.g.
data-testid="mock-server-button" or similar) so Playwright can reliably target
the control; ensure the data-testid is unique and stable across renders.
In `@packages/bruno-app/src/components/StatusBar/StyledWrapper.js`:
- Around line 74-80: Replace the hardcoded background color in the
.mock-server-status-dot styled rule with a theme value from the
styled-components theme prop (e.g., use ${({ theme }) => theme.colors.success}
or the project’s standard status/semantic color key) so the dot color comes from
the theme rather than the literal "#22c55e"; update the rule in StyledWrapper.js
where the .mock-server-status-dot selector is defined to reference the theme
color key used across the app.
In `@packages/bruno-electron/package.json`:
- Around line 52-60: The repo's package-lock.json is out of sync with
package.json causing CI to fail; run npm install (or npm ci after regenerating)
in the project root to regenerate package-lock.json so it matches the
dependencies (e.g., the entries like "cors", "dotenv", "electron-store",
"express" in package.json), verify the lockfile is updated, add/commit the
updated package-lock.json, and push the change so CI can pass.
In `@packages/bruno-electron/src/app/mock-server.js`:
- Around line 269-273: The delayed-response setTimeout started in the request
handler (where setTimeout(sendResponse, delay) is used to defer sendResponse)
can outlive server shutdown and attempt to write to a closed response; track and
clear these timers when the server stops. Modify the handler to store the
returned timeout id (e.g., push it into a Set of activeTimers) and clear that
timer if the response is finished early; then update the server.stop()
implementation to iterate activeTimers, call clearTimeout on each id, and clear
the Set so no pending sendResponse callbacks run after stop(); ensure you still
call sendResponse immediately when delay <= 0 and remove timers when responses
complete.
---
Nitpick comments:
In `@packages/bruno-app/src/components/FilterDropdown/index.js`:
- Around line 75-85: The dropdown options (elements with className
"filter-option" using onClick and calling handleSelect) are not
keyboard-accessible; update the FilterDropdown component to add proper ARIA
roles and keyboard handlers: give the container role="listbox" and each option
role="option", make each option focusable with tabIndex={0}, and add an
onKeyDown handler that calls handleSelect on Enter/Space and moves focus between
options on ArrowUp/ArrowDown (maintain/derive an active index similar to
isActive); ensure the filter-option-check visibility and any active state logic
still reflect keyboard-driven selection.
In `@packages/bruno-app/src/components/FilterDropdown/StyledWrapper.js`:
- Around line 23-24: The styled wrapper uses a hardcoded fallback '#666' inside
rgba(...) instead of a theme-derived default; update both occurrences where
rgba(props.theme.dropdown.selectedColor || '#666', 0.5) is used to reference a
fallback from the theme (e.g., props.theme.colors?.neutral600 or
props.theme.dropdown?.defaultColor) so the component remains fully
theme-managed; locate the expressions in StyledWrapper.js (the styled component
using props.theme.dropdown.selectedColor) and replace the literal '#666' with
the chosen theme fallback key, ensuring the optional chaining matches existing
theme shape and preserves the 0.5 alpha.
In `@packages/bruno-app/src/components/MockServerDashboard/index.js`:
- Around line 63-69: handleDelayChange currently dispatches updateMockDelay on
every keystroke and doesn't surface errors; wrap the dispatch in a short
debounce (e.g., 300ms) so updates are sent only after typing pauses, and call
.unwrap() on the dispatched updateMockDelay promise inside an async handler to
catch/rethrow errors (or handle them with try/catch and dispatch/set an error
state). Update the handleDelayChange (and any helper like a debounced function)
to use the collection.uid and newDelay when invoking
dispatch(updateMockDelay(...)). Ensure the debounced function is memoized (e.g.,
useRef/useCallback) so it isn’t recreated on every render.
- Around line 121-128: The port input mixes number and string types: ensure the
local port state in the MockServerDashboard component is always a number by
initializing port as a number and converting user input to a number in the
onChange handler (use Number(e.target.value) or parseInt) so setPort receives a
numeric value; update the onChange to call setPort(Number(e.target.value)) and
verify any comparisons/uses (e.g., value={isRunning ? serverState.port : port})
assume numeric types.
In `@packages/bruno-app/src/components/MockServerDashboard/RequestLog/index.js`:
- Around line 17-25: The formatTimestamp function forces a US locale by calling
toLocaleTimeString('en-US', ...); update formatTimestamp to use the user's
system locale by passing undefined (or omitting the locale argument) instead of
the hardcoded 'en-US' so the browser uses the runtime locale; ensure the rest of
the options and millisecond padding logic in formatTimestamp remain unchanged
(function name: formatTimestamp).
- Around line 63-68: The current useEffect tied to filteredLogs.length always
forces tableContainerRef to scroll to bottom, which disrupts users who have
scrolled up; modify the effect in RequestLog so it only auto-scrolls when the
container is already near the bottom (e.g., check
tableContainerRef.current.scrollTop + clientHeight >= scrollHeight - THRESHOLD)
or on initial mount, and otherwise preserve the user's scroll position; update
the useEffect that references filteredLogs.length and tableContainerRef to
compute this isNearBottom condition (use a small pixel threshold like 50–150)
before setting scrollTop to scrollHeight.
In
`@packages/bruno-app/src/components/MockServerDashboard/RequestLog/StyledWrapper.js`:
- Around line 52-67: The badge colors for HTTP methods (selectors like &.get,
&.post, &.put, &.patch, &.delete) and status codes (selectors under
.status-code: &.status-2xx, &.status-3xx, &.status-4xx, &.status-5xx,
&.status-unmatched) are hardcoded; update these to use styled-components theme
tokens (via props.theme) instead of hex values so colors follow the global
theme. Replace each hex with the appropriate theme lookup (e.g.,
theme.colors.method.get, theme.colors.status.2xx or your app's badge color
tokens), provide sensible fallbacks if needed, and ensure the selectors in
StyledWrapper.js reference theme values consistently for both method and status
badges.
In `@packages/bruno-app/src/components/MockServerDashboard/RouteTable/index.js`:
- Around line 31-36: The mapping that builds per-row objects currently sets uid
to an index-based string (`uid: \`route-${idx}\``) which breaks React
reconciliation when order changes; update the mapping in the RouteTable
component where you create uid (the block that spreads route and sets uid, hits,
source) to compute a stable unique id from the route itself (for example
`${route.method} ${route.path}` or another stable combination of route
properties) and fall back to a deterministic alternative only if those fields
are missing to ensure uniqueness across rows.
In
`@packages/bruno-app/src/components/MockServerDashboard/RouteTable/StyledWrapper.js`:
- Around line 37-38: Replace hardcoded CSS variables and hex colors in the
styled components with theme values from props.theme: update the border-color in
StyledWrapper (focus state) to use something like props.theme['tabActiveBorder']
(or the correct key from the app theme), and similarly change the method badge's
background and text color definitions (in the MethodBadge/Badge styled
component) to use props.theme keys instead of hex literals or var(--...) so all
route-table colors are driven by props.theme; ensure you reference and use the
exact theme property names used elsewhere in the app and adjust the
styled-components selectors in StyledWrapper.js accordingly.
In `@packages/bruno-app/src/components/MockServerDashboard/StyledWrapper.js`:
- Around line 24-38: The styled component StyledWrapper uses hardcoded hex and
CSS variable colors for states (.running, .starting, .error) and elsewhere;
replace those literal values with theme tokens accessed via the
styled-components theme prop (e.g., props.theme.colors.status.running /
.starting / .error or the appropriate theme keys used across the app) and update
all other hardcoded occurrences (the locations noted around the later blocks) to
use theme.colors.* instead of `#hex` or var(--...). Keep the same class selectors
(.running, .starting, .error) and animation, only swap the color values to the
theme tokens to ensure consistent theming.
In `@packages/bruno-app/src/providers/ReduxStore/slices/mock-server.js`:
- Around line 97-103: The thunk clearMockLog currently awaits
window.ipcRenderer.invoke('renderer:mock-server-clear-log', { collectionUid })
but doesn't inspect the returned result; update clearMockLog to capture the
invoke response (e.g., result) and check result.success like the other thunks,
throwing or returning a rejected value when success is false so failures are
surfaced to Redux and logs; ensure you reference clearMockLog and the
'renderer:mock-server-clear-log' IPC channel when making the change.
In `@packages/bruno-electron/package.json`:
- Around line 60-61: In package.json's dependencies, the "express" entry is out
of alphabetical order between "electron-store" and "electron-util"; reorder the
dependencies so "express" appears after "electron-util" to restore consistent
alphabetical ordering of dependency keys.
In `@packages/bruno-electron/src/app/mock-server.js`:
- Around line 328-336: The stop function may hang because
server.httpServer.close() can wait indefinitely if connections remain open;
update the stop(collectionUid) logic to race the close call against a timeout
and/or call server.httpServer.closeAllConnections() when available: call
server.httpServer.close(...) but wrap it in a Promise that rejects or resolves
after a short timeout (e.g., a few seconds), attempt
server.httpServer.closeAllConnections() before or when the timeout triggers on
Node >=18.2, ensure servers.delete(collectionUid) and emitStatusChanged(...)
still run after the close/timeout so shutdown proceeds reliably.
🪄 Autofix (Beta)
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
Run ID: 588fe846-2773-488f-bbd0-f79001a3061e
📒 Files selected for processing (30)
packages/bruno-app/src/components/FilterDropdown/StyledWrapper.jspackages/bruno-app/src/components/FilterDropdown/index.jspackages/bruno-app/src/components/MockServerDashboard/RequestLog/StyledWrapper.jspackages/bruno-app/src/components/MockServerDashboard/RequestLog/index.jspackages/bruno-app/src/components/MockServerDashboard/RouteTable/StyledWrapper.jspackages/bruno-app/src/components/MockServerDashboard/RouteTable/index.jspackages/bruno-app/src/components/MockServerDashboard/StyledWrapper.jspackages/bruno-app/src/components/MockServerDashboard/index.jspackages/bruno-app/src/components/Preferences/Beta/index.jspackages/bruno-app/src/components/RequestTabPanel/index.jspackages/bruno-app/src/components/RequestTabs/CollectionHeader/index.jspackages/bruno-app/src/components/RequestTabs/RequestTab/SpecialTab.jspackages/bruno-app/src/components/RequestTabs/RequestTab/index.jspackages/bruno-app/src/components/ResponseExample/CreateExampleModal/index.jspackages/bruno-app/src/components/ResponseExample/index.jspackages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/StyledWrapper.jspackages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.jspackages/bruno-app/src/components/Sidebar/Collections/Collection/index.jspackages/bruno-app/src/components/StatusBar/StyledWrapper.jspackages/bruno-app/src/components/StatusBar/index.jspackages/bruno-app/src/providers/App/useIpcEvents.jspackages/bruno-app/src/providers/ReduxStore/index.jspackages/bruno-app/src/providers/ReduxStore/slices/mock-server.jspackages/bruno-app/src/providers/ReduxStore/slices/tabs.jspackages/bruno-app/src/utils/beta-features.jspackages/bruno-electron/package.jsonpackages/bruno-electron/src/app/mock-server.jspackages/bruno-electron/src/index.jspackages/bruno-electron/src/ipc/mock-server.jspackages/bruno-electron/src/store/preferences.js
Comment on lines
+65
to
+71
| <button | ||
| className={`filter-trigger ${isActive ? 'active' : ''}`} | ||
| onClick={handleToggle} | ||
| > | ||
| <span>{selectedLabel}</span> | ||
| <IconChevronDown size={12} strokeWidth={2} className={`filter-chevron ${open ? 'open' : ''}`} /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add type="button" to prevent unintended form submissions.
When nested inside a <form>, a button without an explicit type defaults to "submit", which can cause unwanted form submissions on click.
Proposed fix
<button
+ type="button"
className={`filter-trigger ${isActive ? 'active' : ''}`}
onClick={handleToggle}
>📝 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
| <button | |
| className={`filter-trigger ${isActive ? 'active' : ''}`} | |
| onClick={handleToggle} | |
| > | |
| <span>{selectedLabel}</span> | |
| <IconChevronDown size={12} strokeWidth={2} className={`filter-chevron ${open ? 'open' : ''}`} /> | |
| </button> | |
| <button | |
| type="button" | |
| className={`filter-trigger ${isActive ? 'active' : ''}`} | |
| onClick={handleToggle} | |
| > | |
| <span>{selectedLabel}</span> | |
| <IconChevronDown size={12} strokeWidth={2} className={`filter-chevron ${open ? 'open' : ''}`} /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/FilterDropdown/index.js` around lines 65 -
71, The button element rendered in the FilterDropdown component (the <button>
with className "filter-trigger" that uses selectedLabel, handleToggle and
IconChevronDown) lacks an explicit type and will act as a submit button when
inside a form; update that JSX to include type="button" on the filter-trigger
button to prevent unintended form submissions while preserving the existing
onClick behavior.
Comment on lines
+74
to
+80
| .mock-server-status-dot { | ||
| width: 6px; | ||
| height: 6px; | ||
| border-radius: 50%; | ||
| background: #22c55e; | ||
| display: inline-block; | ||
| } |
Contributor
There was a problem hiding this comment.
Use theme prop for the status dot color.
Hardcoded #22c55e violates coding guidelines. Use a theme-based color for consistency across themes.
🎨 Proposed fix
.mock-server-status-dot {
width: 6px;
height: 6px;
border-radius: 50%;
- background: `#22c55e`;
+ background: ${(props) => props.theme.colors.text.green};
display: inline-block;
}As per coding guidelines: "Use styled component's theme prop to manage CSS colors and not CSS variables when in the context of a styled component."
📝 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
| .mock-server-status-dot { | |
| width: 6px; | |
| height: 6px; | |
| border-radius: 50%; | |
| background: #22c55e; | |
| display: inline-block; | |
| } | |
| .mock-server-status-dot { | |
| width: 6px; | |
| height: 6px; | |
| border-radius: 50%; | |
| background: ${(props) => props.theme.colors.text.green}; | |
| display: inline-block; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/StatusBar/StyledWrapper.js` around lines 74
- 80, Replace the hardcoded background color in the .mock-server-status-dot
styled rule with a theme value from the styled-components theme prop (e.g., use
${({ theme }) => theme.colors.success} or the project’s standard status/semantic
color key) so the dot color comes from the theme rather than the literal
"#22c55e"; update the rule in StyledWrapper.js where the .mock-server-status-dot
selector is defined to reference the theme color key used across the app.
Comment on lines
+269
to
+273
| if (delay > 0) { | ||
| setTimeout(sendResponse, delay); | ||
| } else { | ||
| sendResponse(); | ||
| } |
Contributor
There was a problem hiding this comment.
Pending setTimeout not cancelled on server stop.
If the server is stopped while a delayed response is pending, the callback will still fire and attempt to write to a closed response. Consider tracking active timeouts and clearing them in stop().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/app/mock-server.js` around lines 269 - 273, The
delayed-response setTimeout started in the request handler (where
setTimeout(sendResponse, delay) is used to defer sendResponse) can outlive
server shutdown and attempt to write to a closed response; track and clear these
timers when the server stops. Modify the handler to store the returned timeout
id (e.g., push it into a Set of activeTimers) and clear that timer if the
response is finished early; then update the server.stop() implementation to
iterate activeTimers, call clearTimeout on each id, and clear the Set so no
pending sendResponse callbacks run after stop(); ensure you still call
sendResponse immediately when delay <= 0 and remove timers when responses
complete.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.js (1)
486-500: NormalizestatusCodebefore persisting example data.Line 487 currently stores
statusCodeas-is. IfCreateExampleModalprovides it as a string,example.statuscan become mixed-type across examples. Coerce/validate it once here for consistency.Proposed patch
- const statusCode = mockFields?.statusCode || 200; - const bodyType = mockFields?.bodyType || 'text'; + const parsedStatusCode = Number.parseInt(mockFields?.statusCode, 10); + const statusCode = Number.isInteger(parsedStatusCode) && parsedStatusCode >= 100 && parsedStatusCode <= 599 ? parsedStatusCode : 200; + const bodyType = mockFields?.bodyType === 'json' ? 'json' : 'text'; const defaultContent = bodyType === 'json' ? '{}' : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.js` around lines 486 - 500, The example status is being saved with whatever mockFields.statusCode provides, which can be a string; inside handleCreateExample coerce/validate mockFields.statusCode into a numeric HTTP status before building exampleData (e.g., parse/Number the value and default to 200 if NaN or out of valid range) so exampleData.status is always a number; update the local statusCode assignment and any usage of statusCode within handleCreateExample to use this normalized numeric value.
🤖 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/MockServerDashboard/RouteTable/index.js`:
- Around line 107-114: Add a stable test id to the method filter trigger by
passing a data-testid attribute on the FilterDropdown instance (e.g.
data-testid="method-filter") so the trigger element can be selected by
Playwright; update the FilterDropdown JSX where label="Method" is used to
include data-testid="method-filter" (or if FilterDropdown expects a specific
prop like triggerProps or dataTestId, pass the data-testid through that prop so
the trigger DOM element receives the attribute).
- Around line 31-35: The uid generated in the map (currently `route-${idx}`) is
unstable across filtering; change the uid to a stable unique key based on the
route itself (for example use the same stable key used for hitCounts:
`${route.method} ${route.path}` or a deterministic encoding of that string) so
EditableTable row identity doesn't break; update the mapping where uid is
assigned (the map callback in RouteTable) to set uid = `${route.method}
${route.path}` (or an encoded variant) and ensure no other code expects `route-`
prefixed ids.
In `@packages/bruno-electron/src/app/mock-server.js`:
- Around line 30-52: The extractRoutePath function is stripping variable
segments like {{id}} which prevents matching concrete requests; instead of
removing them, replace each {{name}} with a path parameter placeholder (e.g.,
:name) so routes like /users/{{id}} become /users/:id and will match /users/123.
Update the replacement logic in extractRoutePath (the cleaned variable) to use a
regex replacement of /\{\{([^}]+)\}\}/g -> ':$1' (or another consistent
placeholder format used by your router), then continue with the existing
URL/query cleanup and slash normalization so variable segments are preserved in
the final route key.
- Around line 299-300: The mock server currently calls app.listen(port) which
binds to 0.0.0.0; change the call in the httpServer Promise so app.listen binds
to the loopback interface (e.g., app.listen(port, '127.0.0.1')) to restrict
access to localhost only; update the Promise resolve/reject behavior around the
existing server variable (httpServer, app, port, server) so the server instance
is created with the explicit host parameter and nothing else needs to change.
---
Nitpick comments:
In
`@packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.js`:
- Around line 486-500: The example status is being saved with whatever
mockFields.statusCode provides, which can be a string; inside
handleCreateExample coerce/validate mockFields.statusCode into a numeric HTTP
status before building exampleData (e.g., parse/Number the value and default to
200 if NaN or out of valid range) so exampleData.status is always a number;
update the local statusCode assignment and any usage of statusCode within
handleCreateExample to use this normalized numeric value.
🪄 Autofix (Beta)
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
Run ID: d1730a78-0daf-4fbe-8c82-e7d2f35cd1f5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
packages/bruno-app/src/components/MockServerDashboard/RequestLog/index.jspackages/bruno-app/src/components/MockServerDashboard/RouteTable/index.jspackages/bruno-app/src/components/MockServerDashboard/index.jspackages/bruno-app/src/components/ResponseExample/CreateExampleModal/index.jspackages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.jspackages/bruno-app/src/components/StatusBar/index.jspackages/bruno-electron/src/app/mock-server.jspackages/bruno-electron/src/store/preferences.js
✅ Files skipped from review due to trivial changes (1)
- packages/bruno-electron/src/store/preferences.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/bruno-app/src/components/StatusBar/index.js
- packages/bruno-app/src/components/MockServerDashboard/RequestLog/index.js
- packages/bruno-app/src/components/MockServerDashboard/index.js
- packages/bruno-app/src/components/ResponseExample/CreateExampleModal/index.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/bruno-electron/src/app/mock-server.js (1)
309-313:⚠️ Potential issue | 🟠 MajorClear pending delayed-response timers on stop.
A delayed
setTimeout(sendResponse, delay)can fire after shutdown and attempt to write/log for a stopped server.Suggested fix
@@ servers.set(collectionUid, { collectionUid, collectionPath, port, httpServer, routeMap, globalDelay, - requestLog: [] + requestLog: [], + activeTimers: new Set() }); @@ if (delay > 0) { - setTimeout(sendResponse, delay); + const timer = setTimeout(() => { + server.activeTimers.delete(timer); + sendResponse(); + }, delay); + server.activeTimers.add(timer); + res.on('close', () => { + if (server.activeTimers.delete(timer)) { + clearTimeout(timer); + } + }); } else { sendResponse(); } @@ const stop = async (collectionUid) => { const server = servers.get(collectionUid); if (!server) return; + for (const timer of server.activeTimers) { + clearTimeout(timer); + } + server.activeTimers.clear();Also applies to: 368-384
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/app/mock-server.js` around lines 309 - 313, When scheduling delayed responses with setTimeout(sendResponse, delay) (seen around the sendResponse calls), store the returned timer id (e.g., responseTimer) or push it into a per-server timers array so you can clear it on shutdown; update the stop/close routine to iterate those timer ids and call clearTimeout on each (and optionally set a running flag that sendResponse checks before writing/logging) to prevent delayed callbacks from firing after the server is stopped—apply the same fix to both locations (around lines 309 and 368-384) where delayed setTimeouts are used.
🤖 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/MockServerDashboard/index.js`:
- Around line 177-185: The tab controls are non-focusable divs; update the tab
elements (the ones using getTabClassname('routes') and getTabClassname('log')
that call setActiveTab) to be accessible interactive controls: use semantic
focusable elements (e.g., <button> or add tabIndex={0}), add
aria-selected={activeTab === 'routes'|'log'}, keep role="tab", and add keyboard
handlers (onKeyDown handling Enter/Space to call setActiveTab) so both keyboard
and screen readers can interact with the tabs and the existing
serverState.routeCount and logs badge logic remains unchanged.
In `@packages/bruno-app/src/components/MockServerDashboard/RequestLog/index.js`:
- Around line 17-24: The formatTimestamp function should explicitly handle
invalid Date parsing by checking the constructed Date object's validity: inside
formatTimestamp(iso) create the Date d, then if isNaN(d.getTime()) return the
original iso (or a safe fallback) instead of attempting to format; only call
d.toLocaleTimeString and d.getMilliseconds() when the date is valid so malformed
inputs don't produce broken time text.
In `@packages/bruno-electron/src/app/mock-server.js`:
- Around line 318-325: Add a shutdown guard to prevent new servers from being
started during app quit: introduce a module-level boolean (e.g., isShuttingDown)
that is set true at the start of your quit/stopAll flow (where stopAll()
iterates and stops servers). In the IPC handler for "renderer:mock-server-start"
and at the top of start(collectionUid, ...), check isShuttingDown and
reject/throw immediately (or send an IPC error) so start() cannot insert a new
entry into the servers Map while stopAll()/quit is running; also ensure
stopAll() sets isShuttingDown before iterating and clears it only if appropriate
after shutdown completes.
- Around line 331-335: The CORS config currently uses a wildcard origin in the
app.use(cors(...)) call; replace origin: '*' with a restricted list or a
validation callback that only allows known development origins (e.g.,
http://localhost:<port>, http://127.0.0.1:<port>, or the Electron renderer
origin) so only those origins are accepted while keeping methods and
allowedHeaders as-is; update the app.use(cors(...)) invocation to either provide
an array of allowed origins or an origin function that checks req.headers.origin
and returns true only for permitted local/electron origins.
---
Duplicate comments:
In `@packages/bruno-electron/src/app/mock-server.js`:
- Around line 309-313: When scheduling delayed responses with
setTimeout(sendResponse, delay) (seen around the sendResponse calls), store the
returned timer id (e.g., responseTimer) or push it into a per-server timers
array so you can clear it on shutdown; update the stop/close routine to iterate
those timer ids and call clearTimeout on each (and optionally set a running flag
that sendResponse checks before writing/logging) to prevent delayed callbacks
from firing after the server is stopped—apply the same fix to both locations
(around lines 309 and 368-384) where delayed setTimeouts are used.
🪄 Autofix (Beta)
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
Run ID: 18db1f49-2870-4584-a8f1-bb4a3740cf97
📒 Files selected for processing (7)
packages/bruno-app/src/components/FilterDropdown/index.jspackages/bruno-app/src/components/MockServerDashboard/RequestLog/index.jspackages/bruno-app/src/components/MockServerDashboard/RouteTable/index.jspackages/bruno-app/src/components/MockServerDashboard/StyledWrapper.jspackages/bruno-app/src/components/MockServerDashboard/index.jspackages/bruno-app/src/providers/ReduxStore/middlewares/debug/middleware.jspackages/bruno-electron/src/app/mock-server.js
✅ Files skipped from review due to trivial changes (3)
- packages/bruno-app/src/providers/ReduxStore/middlewares/debug/middleware.js
- packages/bruno-app/src/components/MockServerDashboard/StyledWrapper.js
- packages/bruno-app/src/components/FilterDropdown/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-app/src/components/MockServerDashboard/RouteTable/index.js
Comment on lines
+177
to
+185
| <div className="flex flex-wrap items-center tabs" role="tablist"> | ||
| <div className={getTabClassname('routes')} role="tab" data-testid="mock-server-tab-routes" onClick={() => setActiveTab('routes')}> | ||
| Routes | ||
| {serverState.routeCount > 0 && <sup className="ml-1 font-medium">{serverState.routeCount}</sup>} | ||
| </div> | ||
| <div className={getTabClassname('log')} role="tab" data-testid="mock-server-tab-log" onClick={() => setActiveTab('log')}> | ||
| Request Log | ||
| {logs.length > 0 && <sup className="ml-1 font-medium">{logs.length}</sup>} | ||
| </div> |
Contributor
There was a problem hiding this comment.
Improve tab accessibility for keyboard and screen readers.
Current tab controls are click-only divs. Use proper tab semantics (aria-selected, focusable controls, keyboard-friendly elements like button).
Proposed fix
- <div className="flex flex-wrap items-center tabs" role="tablist">
- <div className={getTabClassname('routes')} role="tab" data-testid="mock-server-tab-routes" onClick={() => setActiveTab('routes')}>
+ <div className="flex flex-wrap items-center tabs" role="tablist">
+ <button
+ className={getTabClassname('routes')}
+ role="tab"
+ aria-selected={activeTab === 'routes'}
+ tabIndex={activeTab === 'routes' ? 0 : -1}
+ type="button"
+ data-testid="mock-server-tab-routes"
+ onClick={() => setActiveTab('routes')}
+ >
Routes
{serverState.routeCount > 0 && <sup className="ml-1 font-medium">{serverState.routeCount}</sup>}
- </div>
- <div className={getTabClassname('log')} role="tab" data-testid="mock-server-tab-log" onClick={() => setActiveTab('log')}>
+ </button>
+ <button
+ className={getTabClassname('log')}
+ role="tab"
+ aria-selected={activeTab === 'log'}
+ tabIndex={activeTab === 'log' ? 0 : -1}
+ type="button"
+ data-testid="mock-server-tab-log"
+ onClick={() => setActiveTab('log')}
+ >
Request Log
{logs.length > 0 && <sup className="ml-1 font-medium">{logs.length}</sup>}
- </div>
+ </button>
</div>📝 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
| <div className="flex flex-wrap items-center tabs" role="tablist"> | |
| <div className={getTabClassname('routes')} role="tab" data-testid="mock-server-tab-routes" onClick={() => setActiveTab('routes')}> | |
| Routes | |
| {serverState.routeCount > 0 && <sup className="ml-1 font-medium">{serverState.routeCount}</sup>} | |
| </div> | |
| <div className={getTabClassname('log')} role="tab" data-testid="mock-server-tab-log" onClick={() => setActiveTab('log')}> | |
| Request Log | |
| {logs.length > 0 && <sup className="ml-1 font-medium">{logs.length}</sup>} | |
| </div> | |
| <div className="flex flex-wrap items-center tabs" role="tablist"> | |
| <button | |
| className={getTabClassname('routes')} | |
| role="tab" | |
| aria-selected={activeTab === 'routes'} | |
| tabIndex={activeTab === 'routes' ? 0 : -1} | |
| type="button" | |
| data-testid="mock-server-tab-routes" | |
| onClick={() => setActiveTab('routes')} | |
| > | |
| Routes | |
| {serverState.routeCount > 0 && <sup className="ml-1 font-medium">{serverState.routeCount}</sup>} | |
| </button> | |
| <button | |
| className={getTabClassname('log')} | |
| role="tab" | |
| aria-selected={activeTab === 'log'} | |
| tabIndex={activeTab === 'log' ? 0 : -1} | |
| type="button" | |
| data-testid="mock-server-tab-log" | |
| onClick={() => setActiveTab('log')} | |
| > | |
| Request Log | |
| {logs.length > 0 && <sup className="ml-1 font-medium">{logs.length}</sup>} | |
| </button> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/MockServerDashboard/index.js` around lines
177 - 185, The tab controls are non-focusable divs; update the tab elements (the
ones using getTabClassname('routes') and getTabClassname('log') that call
setActiveTab) to be accessible interactive controls: use semantic focusable
elements (e.g., <button> or add tabIndex={0}), add aria-selected={activeTab ===
'routes'|'log'}, keep role="tab", and add keyboard handlers (onKeyDown handling
Enter/Space to call setActiveTab) so both keyboard and screen readers can
interact with the tabs and the existing serverState.routeCount and logs badge
logic remains unchanged.
Comment on lines
+17
to
+24
| const formatTimestamp = (iso) => { | ||
| try { | ||
| const d = new Date(iso); | ||
| return d.toLocaleTimeString('en-US', { hour12: false, hour: '2-digit', minute: '2-digit', second: '2-digit' }) | ||
| + '.' + String(d.getMilliseconds()).padStart(3, '0'); | ||
| } catch { | ||
| return iso; | ||
| } |
Contributor
There was a problem hiding this comment.
Handle invalid timestamp inputs explicitly.
Date parsing failures won’t hit catch; invalid values currently render broken time text. Guard with isNaN(d.getTime()) before formatting.
Proposed fix
const formatTimestamp = (iso) => {
- try {
- const d = new Date(iso);
- return d.toLocaleTimeString('en-US', { hour12: false, hour: '2-digit', minute: '2-digit', second: '2-digit' })
- + '.' + String(d.getMilliseconds()).padStart(3, '0');
- } catch {
- return iso;
- }
+ const d = new Date(iso);
+ if (Number.isNaN(d.getTime())) {
+ return iso;
+ }
+ return `${d.toLocaleTimeString('en-US', {
+ hour12: false,
+ hour: '2-digit',
+ minute: '2-digit',
+ second: '2-digit'
+ })}.${String(d.getMilliseconds()).padStart(3, '0')}`;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/MockServerDashboard/RequestLog/index.js`
around lines 17 - 24, The formatTimestamp function should explicitly handle
invalid Date parsing by checking the constructed Date object's validity: inside
formatTimestamp(iso) create the Date d, then if isNaN(d.getTime()) return the
original iso (or a safe fallback) instead of attempting to format; only call
d.toLocaleTimeString and d.getMilliseconds() when the date is valid so malformed
inputs don't produce broken time text.
Comment on lines
+318
to
+325
| const start = async (collectionUid, collectionPath, port = 4000, globalDelay = 0) => { | ||
| if (!preferencesUtil.isBetaFeatureEnabled('mock-server')) { | ||
| throw new Error('Mock server is a beta feature. Enable it in Preferences > Beta.'); | ||
| } | ||
|
|
||
| if (servers.has(collectionUid)) { | ||
| await stop(collectionUid); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify start/stop race surface between IPC and mock-server lifecycle.
rg -n -C3 "renderer:mock-server-start|main:start-quit-flow|stopAll\\(|mockServer.start\\(" packages/bruno-electron/src/ipc/mock-server.js packages/bruno-electron/src/app/mock-server.jsRepository: usebruno/bruno
Length of output: 1555
🏁 Script executed:
wc -l packages/bruno-electron/src/app/mock-server.jsRepository: usebruno/bruno
Length of output: 108
🏁 Script executed:
cat -n packages/bruno-electron/src/app/mock-server.js | sed -n '310,330p'Repository: usebruno/bruno
Length of output: 694
🏁 Script executed:
cat -n packages/bruno-electron/src/app/mock-server.js | sed -n '380,400p'Repository: usebruno/bruno
Length of output: 831
🏁 Script executed:
rg -n "stopAll\(" packages/bruno-electron/src/app/mock-server.js -A 20Repository: usebruno/bruno
Length of output: 40
🏁 Script executed:
rg -n "isShuttingDown|isStopping|quitInProgress" packages/bruno-electron/src/Repository: usebruno/bruno
Length of output: 40
🏁 Script executed:
cat -n packages/bruno-electron/src/app/mock-server.js | sed -n '318,370p'Repository: usebruno/bruno
Length of output: 1957
🏁 Script executed:
rg -n "app.on.*quit|before-quit|will-quit" packages/bruno-electron/src/main.js packages/bruno-electron/src/ipc/mock-server.jsRepository: usebruno/bruno
Length of output: 133
🏁 Script executed:
cat -n packages/bruno-electron/src/ipc/mock-server.js | sed -n '55,70p'Repository: usebruno/bruno
Length of output: 542
Add shutdown guard to prevent concurrent start() during quit flow.
The renderer:mock-server-start IPC handler has no guard against being called during app shutdown. While stopAll() is iterating and stopping servers, a concurrent start() call can insert a new server into the map that never gets stopped. Add a shutdown flag that the IPC handler checks, or prevent renderer IPC calls during the quit phase.
Also applies to: 391-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/app/mock-server.js` around lines 318 - 325, Add a
shutdown guard to prevent new servers from being started during app quit:
introduce a module-level boolean (e.g., isShuttingDown) that is set true at the
start of your quit/stopAll flow (where stopAll() iterates and stops servers). In
the IPC handler for "renderer:mock-server-start" and at the top of
start(collectionUid, ...), check isShuttingDown and reject/throw immediately (or
send an IPC error) so start() cannot insert a new entry into the servers Map
while stopAll()/quit is running; also ensure stopAll() sets isShuttingDown
before iterating and clears it only if appropriate after shutdown completes.
Comment on lines
+331
to
+335
| app.use(cors({ | ||
| origin: '*', | ||
| methods: ['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS', 'HEAD'], | ||
| allowedHeaders: ['Content-Type', 'Authorization', 'X-Mock-Example', 'X-Mock-Response-Code'] | ||
| })); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For browser security: does Access-Control-Allow-Origin: * on a localhost service allow arbitrary websites to read responses, and what are recommended mitigations for local developer tools?
💡 Result:
Yes, setting Access-Control-Allow-Origin: * on a localhost service allows arbitrary websites to read its responses. The wildcard * permits the browser to share the response with JavaScript from any origin, including malicious sites visited by the developer. This enables cross-origin fetches from those sites to localhost, potentially exposing local data, running arbitrary code via dev endpoints, or compromising the developer's machine if the service handles sensitive operations. Recommended mitigations for local developer tools: - Avoid * entirely; instead, explicitly allow specific localhost origins like http://localhost:3000 or https://localhost:3000 (use environment-based allowlists). - Use dev server proxies (e.g., Vite/Webpack proxy) to route API calls through the same origin, bypassing CORS. - Do not combine * with Access-Control-Allow-Credentials: true (browsers block this). - For public non-credentialed data only, * is acceptable but still risky locally. - Add Vary: Origin when dynamically setting origins. - Firewall local services or use auth tokens for dev endpoints. - Prefer no CORS headers if cross-origin access isn't needed.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Access-Control-Allow-Origin
- 2: https://developer.mozilla.org/en-US/docs/Web/Security/Practical_implementation_guides/CORS
- 3: https://stackoverflow.com/questions/12001269/what-security-risks-exist-when-setting-access-control-allow-origin-to-accept-all
- 4: https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CORS
- 5: https://portswigger.net/web-security/cors/access-control-allow-origin
- 6: https://stackoverflow.com/questions/39042799/cors-localhost-as-allowed-origin-in-production
- 7: https://github.blog/security/application-security/localhost-dangers-cors-and-dns-rebinding/
- 8: https://www.simplethread.com/psa-cors-allow-origin-considered-harmful-in-development/
🏁 Script executed:
# First, let's see the mock-server.js file around the flagged lines
head -350 packages/bruno-electron/src/app/mock-server.js | tail -50Repository: usebruno/bruno
Length of output: 1393
🏁 Script executed:
# Also check the beginning of the file to understand what this mock server does
head -50 packages/bruno-electron/src/app/mock-server.jsRepository: usebruno/bruno
Length of output: 1526
🏁 Script executed:
# Search for comments and context around the CORS setup
sed -n '310,360p' packages/bruno-electron/src/app/mock-server.jsRepository: usebruno/bruno
Length of output: 1374
🏁 Script executed:
# Look for where this mock server is instantiated/started to understand its purpose
rg -A 5 -B 5 "mock-server" packages/bruno-electron/src --type js --type jsx | head -100Repository: usebruno/bruno
Length of output: 85
🏁 Script executed:
# Check if there are any docs about the mock server feature
find . -type f \( -name "*.md" -o -name "*.txt" \) | xargs grep -l "mock.*server" 2>/dev/null | head -5Repository: usebruno/bruno
Length of output: 40
🏁 Script executed:
# Find references to the mock server start function
rg "mock-server|mockServer" packages/bruno-electron/src --type js -A 3 -B 3 | head -200Repository: usebruno/bruno
Length of output: 8750
🏁 Script executed:
# Look for API calls to the mock server
rg "localhost.*\d{4}|http://127\.0\.0\.1" packages/bruno-electron/src --type js | head -20Repository: usebruno/bruno
Length of output: 336
🏁 Script executed:
# Check if there's any Electron main/renderer communication for the mock server
grep -r "mock.*server\|mockServer" packages/bruno-electron/src --include="*.js" | grep -E "ipc|window\.|fetch|xhr"Repository: usebruno/bruno
Length of output: 2364
🏁 Script executed:
# Look for the mock server feature documentation or tests
find . -type f \( -name "*.md" -o -name "*.test.js" -o -name "*.spec.js" \) | xargs grep -l "mock.*server" 2>/dev/nullRepository: usebruno/bruno
Length of output: 42
Restrict CORS origins instead of using wildcard.
Wildcard CORS allows any origin to read mock responses. Even though the server binds to 127.0.0.1, restricting origins to specific development origins (e.g., localhost:* or the Electron renderer domain) is safer and still permits local testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/app/mock-server.js` around lines 331 - 335, The
CORS config currently uses a wildcard origin in the app.use(cors(...)) call;
replace origin: '*' with a restricted list or a validation callback that only
allows known development origins (e.g., http://localhost:<port>,
http://127.0.0.1:<port>, or the Electron renderer origin) so only those origins
are accepted while keeping methods and allowedHeaders as-is; update the
app.use(cors(...)) invocation to either provide an array of allowed origins or
an origin function that checks req.headers.origin and returns true only for
permitted local/electron origins.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/mocker/collection-mocker.spec.ts (6)
462-474: ReplacewaitForTimeoutwith auto-retrying assertion.Same concern at line 468 — a fixed wait can cause flakiness or unnecessary slowdowns.
Alternative
await mockFetch('/health'); await openMockerTab(page); await page.getByTestId('mock-server-tab-log').click(); - await page.waitForTimeout(500); const logCount = page.getByTestId('mock-server-log-count'); - await expect(logCount).toBeVisible(); - const text = await logCount.innerText(); - expect(parseInt(text)).toBe(1); + await expect(logCount).toHaveText('1', { timeout: 5000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mocker/collection-mocker.spec.ts` around lines 462 - 474, The test 'should accumulate new log entries after clearing' uses a fixed sleep via page.waitForTimeout(500); replace that with an auto-retrying assertion against the DOM node obtained by page.getByTestId('mock-server-log-count') — e.g. await expect(logCount).toHaveText(/1/) or another Playwright retriable matcher so the test waits until the count updates instead of sleeping; remove the waitForTimeout call and rely on the retriable expectation to detect the new log entry.
97-98: Consider usingdata-testidfor table row locators.These CSS selectors (
.table-container table tbody tr) are more fragile thandata-testidattributes. If the markup changes, these tests will break.As per coding guidelines: "Add
data-testidto testable elements for Playwright."Example improvement
- const rows = page.locator('.table-container table tbody tr'); + const rows = page.getByTestId('mock-server-route-row');This requires adding
data-testid="mock-server-route-row"to the route table rows inRouteTable/index.js.Also applies to: 103-104, 114-115, 121-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mocker/collection-mocker.spec.ts` around lines 97 - 98, Replace fragile CSS row selectors in the Playwright tests with a stable data-testid: add data-testid="mock-server-route-row" to the table row element in the route table component (RouteTable/index.js) where rows are rendered, then update the tests that currently use ".table-container table tbody tr" (and the other occurrences at the referenced blocks) to locate rows via page.locator('[data-testid="mock-server-route-row"]') and assertions like toHaveCount; this ensures tests target the element by the unique identifier instead of brittle CSS structure.
355-368: Multiple CSS class selectors could benefit fromdata-testid.Locators like
.log-table-container .method-badge,.log-path,.no-match-label,.status-codeare implementation-dependent. Addingdata-testidattributes to these elements in the component would make these tests more robust.As per coding guidelines: "Add
data-testidto testable elements for Playwright."Also applies to: 381-381, 394-394, 398-398, 410-410, 428-428
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mocker/collection-mocker.spec.ts` around lines 355 - 368, The tests use fragile CSS selectors like '.log-table-container .method-badge', '.log-table-container .log-path', '.no-match-label', and '.status-code' (e.g. in the "Log entries should contain matched request data" test step) which are implementation-dependent; modify the UI components that render these elements to include stable data-testid attributes (for example data-testid="method-badge", data-testid="log-path", data-testid="no-match-label", data-testid="status-code"), then update the spec to use Playwright locators by these data-testid values (e.g. page.locator('[data-testid="method-badge"]')) so tests are robust to styling/structure changes.
530-536: Good cross-platform handling, but reduce the hard wait.Line 530 correctly handles macOS vs other platforms for keyboard shortcuts — nice attention to cross-platform compatibility.
However, line 536's
waitForTimeout(500)could be replaced with an assertion that the save operation completed (e.g., waiting for a toast or the edit button to reappear).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mocker/collection-mocker.spec.ts` around lines 530 - 536, After clicking the save button (response-example-save-btn) replace the hard waitForTimeout(500) with an explicit wait/assertion that the save completed: wait for a visible success indicator such as a toast or the edit button reappearing (e.g., waitForSelector or getByTestId('response-example-edit-btn')/getByTestId('toast-success') and assert visible/attached), using a reasonable timeout so the test is deterministic across platforms.
574-581: Use a clearer pattern for expected connection failure.
expect(true).toBe(false)is unconventional and the failure message won't be descriptive. Consider usingexpect.fail()or a more explicit assertion.Cleaner approach
test('should become unreachable after stop', async () => { - try { - await fetch(`${MOCK_BASE}/health`, { signal: AbortSignal.timeout(2000) }); - expect(true).toBe(false); - } catch { - // Expected -- connection refused - } + await expect(async () => { + await fetch(`${MOCK_BASE}/health`, { signal: AbortSignal.timeout(2000) }); + }).rejects.toThrow(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mocker/collection-mocker.spec.ts` around lines 574 - 581, The test "should become unreachable after stop" uses expect(true).toBe(false) which yields an unclear failure; replace the try/catch pattern with an explicit failing assertion: either use await expect(fetch(`${MOCK_BASE}/health`, { signal: AbortSignal.timeout(2000) })).rejects.toThrow() to assert the fetch rejects, or keep the try/catch but call expect.fail('expected connection to be refused') (or similar message) inside the try block; update the test body accordingly where the fetch call is made so failures are reported clearly.
376-384: MinimizewaitForTimeoutusage.Line 380 uses a fixed 500ms wait. Per guidelines, try to reduce
page.waitForTimeout()unless absolutely necessary. Consider polling for the expected state instead.Alternative using expect with auto-retry
await test.step('Verify No Match label appears in log', async () => { await openMockerTab(page); await page.getByTestId('mock-server-tab-log').click(); - // Wait for the log to update - await page.waitForTimeout(500); - const noMatchLabels = page.locator('.log-table-container .no-match-label'); - expect(await noMatchLabels.count()).toBeGreaterThan(0); + // Let Playwright auto-retry until the label appears + const noMatchLabel = page.locator('.log-table-container .no-match-label').first(); + await expect(noMatchLabel).toBeVisible({ timeout: 5000 }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mocker/collection-mocker.spec.ts` around lines 376 - 384, The test currently uses a fixed wait (page.waitForTimeout(500)) after clicking the log tab; remove that timeout and instead wait for the log element to appear using Playwright's auto-retrying assertions. Replace the timeout with an assertion like await expect(noMatchLabels.first()).toBeVisible() (keeping the locator defined as const noMatchLabels = page.locator('.log-table-container .no-match-label')), leaving openMockerTab(page) and page.getByTestId('mock-server-tab-log').click() unchanged so the test reliably waits for the "No Match" label without an arbitrary sleep.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/mocker/collection-mocker.spec.ts`:
- Around line 480-502: The timing assertions in the tests "should apply global
delay to matched responses" and "should not delay 404 responses" that use
Date.now() around mockFetch are flaky; update the tests to avoid strict
wall-clock checks by either (a) increasing tolerance margins (e.g., expect
>=300ms for 500ms delay and <800ms for non-delayed 404) or preferably (b) modify
the mock server response to include a diagnostic header (e.g., X-Mock-Delay or
similar) and assert that header is present/absent after calling
mockFetch('/health') and mockFetch('/nonexistent'); locate the checks in the
tests named exactly "should apply global delay to matched responses" and "should
not delay 404 responses" and update the assertions accordingly, and if adding a
header you may need to adjust the mock server/mocking utility used by mockFetch
to set that header when applying global delay.
---
Nitpick comments:
In `@tests/mocker/collection-mocker.spec.ts`:
- Around line 462-474: The test 'should accumulate new log entries after
clearing' uses a fixed sleep via page.waitForTimeout(500); replace that with an
auto-retrying assertion against the DOM node obtained by
page.getByTestId('mock-server-log-count') — e.g. await
expect(logCount).toHaveText(/1/) or another Playwright retriable matcher so the
test waits until the count updates instead of sleeping; remove the
waitForTimeout call and rely on the retriable expectation to detect the new log
entry.
- Around line 97-98: Replace fragile CSS row selectors in the Playwright tests
with a stable data-testid: add data-testid="mock-server-route-row" to the table
row element in the route table component (RouteTable/index.js) where rows are
rendered, then update the tests that currently use ".table-container table tbody
tr" (and the other occurrences at the referenced blocks) to locate rows via
page.locator('[data-testid="mock-server-route-row"]') and assertions like
toHaveCount; this ensures tests target the element by the unique identifier
instead of brittle CSS structure.
- Around line 355-368: The tests use fragile CSS selectors like
'.log-table-container .method-badge', '.log-table-container .log-path',
'.no-match-label', and '.status-code' (e.g. in the "Log entries should contain
matched request data" test step) which are implementation-dependent; modify the
UI components that render these elements to include stable data-testid
attributes (for example data-testid="method-badge", data-testid="log-path",
data-testid="no-match-label", data-testid="status-code"), then update the spec
to use Playwright locators by these data-testid values (e.g.
page.locator('[data-testid="method-badge"]')) so tests are robust to
styling/structure changes.
- Around line 530-536: After clicking the save button
(response-example-save-btn) replace the hard waitForTimeout(500) with an
explicit wait/assertion that the save completed: wait for a visible success
indicator such as a toast or the edit button reappearing (e.g., waitForSelector
or getByTestId('response-example-edit-btn')/getByTestId('toast-success') and
assert visible/attached), using a reasonable timeout so the test is
deterministic across platforms.
- Around line 574-581: The test "should become unreachable after stop" uses
expect(true).toBe(false) which yields an unclear failure; replace the try/catch
pattern with an explicit failing assertion: either use await
expect(fetch(`${MOCK_BASE}/health`, { signal: AbortSignal.timeout(2000)
})).rejects.toThrow() to assert the fetch rejects, or keep the try/catch but
call expect.fail('expected connection to be refused') (or similar message)
inside the try block; update the test body accordingly where the fetch call is
made so failures are reported clearly.
- Around line 376-384: The test currently uses a fixed wait
(page.waitForTimeout(500)) after clicking the log tab; remove that timeout and
instead wait for the log element to appear using Playwright's auto-retrying
assertions. Replace the timeout with an assertion like await
expect(noMatchLabels.first()).toBeVisible() (keeping the locator defined as
const noMatchLabels = page.locator('.log-table-container .no-match-label')),
leaving openMockerTab(page) and page.getByTestId('mock-server-tab-log').click()
unchanged so the test reliably waits for the "No Match" label without an
arbitrary sleep.
🪄 Autofix (Beta)
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
Run ID: d2447efb-7963-4b9c-9a54-8a786aec3191
📒 Files selected for processing (6)
tests/mocker/collection-mocker.spec.tstests/mocker/fixtures/collection/bruno.jsontests/mocker/fixtures/collection/health/healthcheck.brutests/mocker/fixtures/collection/users/create-user.brutests/mocker/fixtures/collection/users/get-user.brutests/mocker/init-user-data/preferences.json
✅ Files skipped from review due to trivial changes (5)
- tests/mocker/fixtures/collection/bruno.json
- tests/mocker/init-user-data/preferences.json
- tests/mocker/fixtures/collection/users/create-user.bru
- tests/mocker/fixtures/collection/health/healthcheck.bru
- tests/mocker/fixtures/collection/users/get-user.bru
Comment on lines
+480
to
+502
| test('should apply global delay to matched responses', async ({ pageWithUserData: page }) => { | ||
| await openMockerTab(page); | ||
| await page.getByTestId('mock-server-delay-input').fill('500'); | ||
|
|
||
| const start = Date.now(); | ||
| await mockFetch('/health'); | ||
| const elapsed = Date.now() - start; | ||
| expect(elapsed).toBeGreaterThanOrEqual(400); | ||
|
|
||
| await page.getByTestId('mock-server-delay-input').fill('0'); | ||
| }); | ||
|
|
||
| test('should not delay 404 responses', async ({ pageWithUserData: page }) => { | ||
| await openMockerTab(page); | ||
| await page.getByTestId('mock-server-delay-input').fill('1000'); | ||
|
|
||
| const start = Date.now(); | ||
| await mockFetch('/nonexistent'); | ||
| const elapsed = Date.now() - start; | ||
| expect(elapsed).toBeLessThan(500); | ||
|
|
||
| await page.getByTestId('mock-server-delay-input').fill('0'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Timing-based assertions are inherently flaky.
Lines 487 and 499 rely on Date.now() measurements. Under CI load or slow environments, these can produce false negatives. The 100ms tolerance (expecting ≥400ms for a 500ms delay) helps, but consider:
- Increasing the tolerance margin further, or
- Using a server-side mechanism to confirm the delay was applied (e.g., a header or log entry indicating delay).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/mocker/collection-mocker.spec.ts` around lines 480 - 502, The timing
assertions in the tests "should apply global delay to matched responses" and
"should not delay 404 responses" that use Date.now() around mockFetch are flaky;
update the tests to avoid strict wall-clock checks by either (a) increasing
tolerance margins (e.g., expect >=300ms for 500ms delay and <800ms for
non-delayed 404) or preferably (b) modify the mock server response to include a
diagnostic header (e.g., X-Mock-Delay or similar) and assert that header is
present/absent after calling mockFetch('/health') and mockFetch('/nonexistent');
locate the checks in the tests named exactly "should apply global delay to
matched responses" and "should not delay 404 responses" and update the
assertions accordingly, and if adding a header you may need to adjust the mock
server/mocking utility used by mockFetch to set that header when applying global
delay.
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
1 participant
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
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