feat: bruno with xmas hat #6362
Conversation
sanish-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughElectron now detects holidays and resolves holiday-specific icons, sends Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Electron Main
participant FS as Filesystem
participant IPC as IPC Bridge
participant Renderer as Renderer (Bruno UI)
participant Redux as Redux Store
participant React as React Components
Main->>FS: getCurrentHoliday() / check date ranges
FS-->>Main: returns holiday name or null
Main->>FS: getHolidayIconPath(baseIcon)
FS-->>Main: returns holiday-specific icon path or default
Main->>Main: create BrowserWindow with windowIconPath
Main->>Main: set macOS dock icon if applicable
Main->>IPC: send "main:app-loaded" { currentHoliday, ... }
IPC->>Renderer: init payload arrives
Renderer->>Redux: dispatch(setCurrentHoliday(currentHoliday))
Redux->>Redux: update state.app.currentHoliday
Redux->>React: state update -> re-render
React->>React: AppTitleBar renders BrunoXmas if 'xmas' else Bruno
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/bruno-electron/src/utils/holidays.js (2)
52-72: Moverequirestatements to module level.Calling
require('fs')andrequire('path')inside functions adds overhead on each invocation. These are core Node.js modules and should be required once at the top of the file.Also, the extension parsing (Line 60-61) using
split('.').pop()can misbehave for filenames with multiple dots (e.g.,icon.backup.png) or no extension.+const fs = require('fs'); +const path = require('path'); + /** * Holiday detection utility ... function getHolidayIconPath(basePath, defaultIcon) { const holiday = getCurrentHoliday(); - const fs = require('fs'); - const path = require('path'); if (holiday) { - const ext = defaultIcon.split('.').pop(); - const nameWithoutExt = defaultIcon.replace(`.${ext}`, ''); + const ext = path.extname(defaultIcon); + const nameWithoutExt = path.basename(defaultIcon, ext); - const holidayIcon = `${nameWithoutExt}-${holiday}.${ext}`; + const holidayIcon = `${nameWithoutExt}-${holiday}${ext}`;
81-102: Duplicate code withgetHolidayIconPath.This function shares nearly identical logic with
getHolidayIconPath. Consider extracting a shared helper to reduce duplication.Additionally, the JSDoc comment (Line 88) says
icon-christmas.icnsbut the actual suffix would beicon-xmas.icnsbased ongetCurrentHoliday()returning'xmas'.- // e.g., icon.icns -> icon-christmas.icns + // e.g., icon.icns -> icon-xmas.icnspackages/bruno-app/src/components/Bruno/xmas.js (1)
7-9: Consider adding a default value for thewidthprop.If
widthis not provided, the SVG dimensions will be undefined. Adding a default ensures consistent rendering.Apply this diff to add a default:
-const BrunoChristmas = ({ width }) => { +const BrunoChristmas = ({ width = 24 }) => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/bruno-electron/resources/icons/png/256x256-xmas.pngis excluded by!**/*.pngpackages/bruno-electron/resources/icons/win/icon-xmas.icois excluded by!**/*.icopackages/bruno-electron/src/about/256x256-xmas.pngis excluded by!**/*.png
📒 Files selected for processing (6)
packages/bruno-app/src/components/AppTitleBar/index.js(3 hunks)packages/bruno-app/src/components/Bruno/xmas.js(1 hunks)packages/bruno-app/src/pages/Bruno/index.js(3 hunks)packages/bruno-app/src/providers/ReduxStore/slices/app.js(3 hunks)packages/bruno-electron/src/index.js(4 hunks)packages/bruno-electron/src/utils/holidays.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/Bruno/xmas.jspackages/bruno-app/src/components/AppTitleBar/index.jspackages/bruno-electron/src/index.jspackages/bruno-electron/src/utils/holidays.jspackages/bruno-app/src/providers/ReduxStore/slices/app.jspackages/bruno-app/src/pages/Bruno/index.js
🧬 Code graph analysis (3)
packages/bruno-electron/src/index.js (1)
packages/bruno-electron/src/utils/arch.js (2)
isMac(2-2)getIsRunningInRosetta(1-14)
packages/bruno-electron/src/utils/holidays.js (1)
packages/bruno-electron/src/index.js (13)
require(14-14)require(15-15)require(16-16)require(34-34)require(46-46)require(47-47)require(51-51)require(52-52)require(53-53)require(56-56)require(57-57)require(93-93)require(122-122)
packages/bruno-app/src/pages/Bruno/index.js (16)
packages/bruno-app/src/components/AppTitleBar/index.js (1)
dispatch(24-24)packages/bruno-app/src/components/Sidebar/SidebarHeader/index.js (1)
dispatch(36-36)packages/bruno-app/src/components/Sidebar/index.js (1)
dispatch(19-19)packages/bruno-app/src/providers/App/useIpcEvents.js (1)
dispatch(36-36)packages/bruno-app/src/components/StatusBar/index.js (1)
dispatch(18-18)packages/bruno-app/src/components/Devtools/Console/index.js (1)
dispatch(302-302)packages/bruno-app/src/components/FolderSettings/Headers/index.js (1)
dispatch(17-17)packages/bruno-app/src/components/RequestPane/RequestHeaders/index.js (1)
dispatch(20-20)packages/bruno-app/src/components/RequestPane/GraphQLVariables/index.js (1)
dispatch(14-14)packages/bruno-app/src/components/CollectionSettings/Script/index.js (1)
dispatch(12-12)packages/bruno-app/src/components/FolderSettings/Script/index.js (1)
dispatch(12-12)packages/bruno-app/src/components/RequestPane/Script/index.js (1)
dispatch(11-11)packages/bruno-app/src/components/CollectionSettings/Tests/index.js (1)
dispatch(11-11)packages/bruno-app/src/components/Devtools/Console/DebugTab/index.js (1)
dispatch(61-61)packages/bruno-app/src/components/Devtools/Console/NetworkTab/index.js (1)
dispatch(213-213)packages/bruno-app/src/components/RequestPane/MultipartFormParams/index.js (1)
dispatch(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Unit Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
🔇 Additional comments (12)
packages/bruno-app/src/providers/ReduxStore/slices/app.js (2)
49-51: LGTM!The
currentHolidaystate property is well-placed and follows existing patterns. The initialnullvalue correctly indicates no holiday by default.Minor note: The comment says
'christmas'but the actual value used is'xmas'(perholidays.js). Consider updating the comment for consistency.- currentHoliday: null // Current holiday theme (e.g., 'christmas') + currentHoliday: null // Current holiday theme (e.g., 'xmas')
124-126: LGTM!Simple, clean reducer that correctly updates the holiday state.
packages/bruno-electron/src/utils/holidays.js (1)
10-26: LGTM!The Christmas period logic is straightforward and correctly covers December through Epiphany (January 6). Good use of JSDoc comments.
packages/bruno-app/src/components/AppTitleBar/index.js (3)
13-13: LGTM!Import follows the existing component import pattern.
51-51: LGTM!Selector is minimal and correctly retrieves the holiday state.
209-213: LGTM!Clean conditional rendering. Both components receive consistent props.
packages/bruno-electron/src/index.js (3)
112-138: LGTM! Solid error handling.Good defensive coding with the
try/catchandisEmpty()check for the dock icon. The conditional require ofnativeImageinside the macOS block is acceptable.Minor: The comments reference
-christmassuffix (Lines 113, 120) but the actual suffix fromgetCurrentHoliday()is-xmas. Consider aligning comments with implementation.
155-155: LGTM!Correctly using the dynamic
windowIconPathinstead of a hardcoded path.
262-265: LGTM!Clean extension of the payload to include
currentHoliday. The renderer will receive this on app load.packages/bruno-app/src/pages/Bruno/index.js (2)
11-13: LGTM!Imports are correctly added for the new holiday dispatch functionality.
87-95: LGTM!The conditional dispatch correctly initializes the holiday state from the Electron main process. The guard
if (init.currentHoliday)ensures no dispatch occurs when there's no active holiday.Note:
dispatchis omitted from the dependency array (Line 95). This is fine since Redux's dispatch is stable, but if your linter enforcesexhaustive-deps, you may want to include it.packages/bruno-app/src/components/Bruno/xmas.js (1)
1-27: LGTM overall!The component is well-structured, follows coding guidelines, and achieves the goal of providing a Christmas-themed Bruno logo. The JSDoc comment is helpful, and the implementation integrates cleanly with the holiday detection system.
|
@Pragadesh44-Bruno can you review this? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/bruno-electron/src/index.js (2)
112-140: Dynamic holiday icon & dock icon logic looks solid; consider a tiny reuse/refactorThis block is well‑scoped to
app.on('ready'), guarded byisMac && app.dock, checks file existence, and wrapsnativeImageusage in atry/catch, which is exactly what you want in Electron main.Two small, optional cleanups you could consider:
- Reuse the already computed
windowIconPathfor the dock (and just guard withfs.existsSync(windowIconPath)), instead of recomputing the same path.- If there’s a dedicated helper for dock icons (e.g. to pick different assets for macOS), you might route the dock path through that to make intent clearer.
Functionally this is good as‑is; these are just polish suggestions.
263-266: Extendingmain:app-loadedpayload withcurrentHolidayis a good integration pointAdding
currentHolidaynext toisRunningInRosettaat the singlemain:app-loadedemit keeps the renderer contract tidy and makes it easy for React/Redux to pick up holiday state.Just make sure all listeners on
main:app-loadedin the renderer treatcurrentHolidayas optional for safety (in case of mixed-version environments).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-electron/src/index.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-electron/src/index.js
🧬 Code graph analysis (1)
packages/bruno-electron/src/index.js (2)
packages/bruno-electron/src/utils/holidays.js (4)
path(55-55)path(84-84)fs(54-54)fs(83-83)packages/bruno-electron/src/utils/arch.js (1)
isMac(2-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - macOS
🔇 Additional comments (2)
packages/bruno-electron/src/index.js (2)
57-57: Holiday utilities import looks consistent with existing architectureImporting holiday helpers into the main process keeps icon logic centralized and matches how other utilities are wired in this file. No issues here.
156-156: WiringwindowIconPathintoBrowserWindowis appropriateUsing the computed
windowIconPathhere keeps the holiday icon selection in one place and doesn’t change the surrounding window options. Behavior should remain the same on non‑holiday days if your helper falls back correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/bruno-electron/src/index.js (1)
121-138: PrefergetHolidayDockIconPathand consider a sane default for the dock iconYou already import
getHolidayDockIconPath, and the helper is documented as being tailored for Dock icons. Using it here keeps intent clear and lets you customize Dock-specific assets later without touching this file. You can also mirror the window-icon fallback so the dock stays on-brand outside holidays:- if (isMac && app.dock) { - // Use the same PNG as window icon - it's in the app bundle and works reliably - const dockIconPath = getHolidayIconPath( - path.join(__dirname, 'about'), - '256x256.png' - ); + if (isMac && app.dock) { + // Use a holiday-specific Dock icon when available, otherwise fall back to the default PNG + const dockIconPath = getHolidayDockIconPath( + path.join(__dirname, 'about'), + '256x256.png' + ) || path.join(__dirname, 'about', '256x256.png');The rest of the
fs.existsSync/nativeImage/setIconflow looks solid.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-electron/src/index.js(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-electron/src/index.js
🧬 Code graph analysis (1)
packages/bruno-electron/src/index.js (2)
packages/bruno-electron/src/utils/holidays.js (2)
path(55-55)path(84-84)packages/bruno-electron/src/utils/arch.js (1)
isMac(2-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - Linux
🔇 Additional comments (2)
packages/bruno-electron/src/index.js (2)
15-15: Holiday helpers and native image wiring look correctThe added
nativeImageand holiday utility imports are consistent with their later usage for icons and payload; nothing to change here.Also applies to: 57-57
262-265:currentHolidayinmain:app-loadedpayload looks good; ensure renderer typing is updatedPlumbing
currentHoliday: getCurrentHoliday()into themain:app-loadedpayload is a clean way to expose this to the renderer. Just make sure any TypeScript types/interfaces (and Redux slice typings) that describe this IPC payload are updated to includecurrentHoliday(string or null) so consumers don’t rely on it being absent.
| // Get dynamic icons based on holiday | ||
| // Window icon - uses naming convention: 256x256-xmas.png | ||
| const windowIconPath = getHolidayIconPath( | ||
| path.join(__dirname, 'about'), | ||
| '256x256.png' | ||
| ); | ||
|
|
There was a problem hiding this comment.
Add a fallback so the window icon stays Bruno-branded when no holiday is active
As written, getHolidayIconPath can return undefined when there’s no active holiday, which would leave icon: windowIconPath without a value and likely fall back to the generic Electron/OS icon instead of Bruno’s normal icon.
You can keep the existing behavior outside holiday periods by falling back to the default icon path:
- // Window icon - uses naming convention: 256x256-xmas.png
- const windowIconPath = getHolidayIconPath(
- path.join(__dirname, 'about'),
- '256x256.png'
- );
+ // Window icon - uses naming convention: 256x256-xmas.png
+ const windowIconPath = getHolidayIconPath(
+ path.join(__dirname, 'about'),
+ '256x256.png'
+ ) || path.join(__dirname, 'about', '256x256.png');This keeps the Bruno icon in non-holiday periods while still swapping to the holiday variant when available.
Also applies to: 155-155
🤖 Prompt for AI Agents
In packages/bruno-electron/src/index.js around lines 112-118 (and also at 155),
getHolidayIconPath can return undefined leaving the window icon empty; define a
stable default Bruno icon path (e.g., path.join(__dirname, 'about',
'256x256.png') or central DEFAULT_ICON_PATH) and use it as a fallback when
getHolidayIconPath returns falsy so the window/icon option always receives a
valid path (assign windowIconPath = getHolidayIconPath(...) || DEFAULT_ICON_PATH
and use DEFAULT_ICON_PATH at the other occurrence).
There was a problem hiding this comment.
the function under the hood return the original path, when it is not holiday
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# View the implementation of getHolidayIconPath function
cat packages/bruno-electron/src/utils/holidays.js
Length of output: 3020
@sanish-bruno You're absolutely right! Looking at the implementation of getHolidayIconPath, it already has a fallback built in at the end:
// Fall back to default icon
return path.join(basePath, defaultIcon);So the function will always return a valid path - either the holiday-specific icon if one exists during a holiday period, or the default icon otherwise. The additional fallback suggested in the review comment isn't necessary. Thanks for the clarification!
🧬 Code Graph Analysis Results
[summary of potentially relevant code snippets]
- File: packages/bruno-electron/src/index.js
- Location: near the startup/bootstrap logic
- Summary: The code uses holiday-related utilities to determine icons:
- getHolidayIconPath(baseDir, fileName) is invoked to compute the window icon path (windowIconPath). If a holiday-based icon is available, this path points to it.
- getHolidayDockIconPath(baseDir, fileName) is invoked (when on macOS) to compute the dock icon path (dockIconPath) for holiday-specific icons.
- getCurrentHoliday() is invoked to retrieve the currently active holiday, and this value is sent to the renderer via main:app-loaded payload.
- Purpose relative to user comment: These functions appear to under-the-hood return holiday-specific assets/metadata; behavior when not holiday would fall back to default (non-holiday) assets, aligning with the idea of returning the “original path” when it is not holiday.
Notes:
- The above describes how the file interacts with holiday utilities and what values are produced, which is relevant to understanding how “original path” vs. holiday-specific path is resolved. No direct code snippets were provided for the holiday utilities themselves in the supplied snippets.
|
Useless, therefore indispensable. |
2 participants
Description
This adds bruno with the xmas hat to the title bar and dock icon
Mac
windows
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
✏️ Tip: You can customize this high-level summary in your review settings.