Repository navigation
chore(app): fix initial app load using code splitting and lazy load - #8626
sachin-thakur-bruno wants to merge 6 commits into
Conversation
sachin-thakur-bruno
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR extracts response formatting, code-splits heavy application components and previews, scopes editor and resource initialization, adds Electron bundle settings, and removes the terminal session ID helper. ChangesApplication loading and rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/utils/common/index.js (1)
301-317: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate
prettifyHtmlStringacross two files, already diverging.formatResponseandprettifyJavaScriptStringwere properly removed fromutils/common/index.jsand replaced with "moved" comments, butprettifyHtmlStringwas left behind there while also being re-implemented in the new module — and the two copies already differ (one still logs errors, the other doesn't). SinceprettifyHtmlStringdoesn't useprettier/fast-json-format, there's no bundle-splitting reason to duplicate it; it should have a single source of truth.
packages/bruno-app/src/utils/common/index.js#L301-L317: keep this as the single implementation (it's already exported from this module).packages/bruno-app/src/utils/common/format-response.js#L31-L43: remove this copy and instead importprettifyHtmlStringfromutils/commonalongsidesafeStringifyJSON,safeParseXML,formatHexView,isHexFormat.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/utils/common/index.js` around lines 301 - 317, Use the existing exported prettifyHtmlString implementation in packages/bruno-app/src/utils/common/index.js as the single source of truth. In packages/bruno-app/src/utils/common/format-response.js, remove the local duplicate and import prettifyHtmlString from utils/common alongside safeStringifyJSON, safeParseXML, formatHexView, and isHexFormat; no direct change is needed in index.js.
🧹 Nitpick comments (1)
packages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultPreview/PdfPreview.js (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid variable shadowing.
The callback parameter
{ numPages }shadows thenumPagesstate variable defined on line 17. While not functionally broken, it's a confusing practice. Consider renaming the parameter to improve clarity.♻️ Proposed refactor
- onLoadSuccess={({ numPages }) => setNumPages(numPages)} + onLoadSuccess={(pdf) => setNumPages(pdf.numPages)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultPreview/PdfPreview.js` at line 23, Rename the onLoadSuccess callback’s destructured numPages parameter in PdfPreview to avoid shadowing the existing numPages state variable, and update the setNumPages call to use the renamed parameter while preserving the current behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultPreview/PdfPreview.js`:
- Around line 25-27: Update the page iteration in the PDF preview component to
use Array.from with a length of numPages || 0 instead of new Array(numPages),
ensuring the initial null state produces no Page elements while preserving the
existing page numbering and rendering props.
In `@packages/bruno-app/src/pages/Bruno/index.js`:
- Around line 127-133: Replace the default loading labels with accurate name
props on every affected fallback: in packages/bruno-app/src/pages/Bruno/index.js
at lines 127-133, pass API Spec and workspace names to the two PageLoader
instances; at lines 151-153 and 156-158, pass AI Assistant. In
packages/bruno-app/src/components/RequestTabPanel/index.js at lines 427-431,
530-534, and 538-542, pass changelog, OpenAPI Sync, and OpenAPI Spec
respectively to RequestTabPanelLoading.
- Around line 163-165: Update the Suspense wrapper around Devtools to use a null
fallback instead of PageLoader, preserving the bottom-anchored panel’s layout
while its chunk loads. Leave the Devtools rendering and mainSectionRef prop
unchanged.
In `@packages/bruno-app/src/utils/codemirror/setup.js`:
- Around line 9-41: Add the companion CSS imports for the CodeMirror addons
loaded in setup.js: dialog, foldgutter, show-hint, and lint. Place them with the
existing addon CSS imports while leaving the JavaScript addon requires
unchanged.
---
Outside diff comments:
In `@packages/bruno-app/src/utils/common/index.js`:
- Around line 301-317: Use the existing exported prettifyHtmlString
implementation in packages/bruno-app/src/utils/common/index.js as the single
source of truth. In packages/bruno-app/src/utils/common/format-response.js,
remove the local duplicate and import prettifyHtmlString from utils/common
alongside safeStringifyJSON, safeParseXML, formatHexView, and isHexFormat; no
direct change is needed in index.js.
---
Nitpick comments:
In
`@packages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultPreview/PdfPreview.js`:
- Line 23: Rename the onLoadSuccess callback’s destructured numPages parameter
in PdfPreview to avoid shadowing the existing numPages state variable, and
update the setNumPages call to use the renamed parameter while preserving the
current behavior.
🪄 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: 0c880ff7-f06b-4bba-b71e-b7a22bcfc91b
📒 Files selected for processing (19)
packages/bruno-app/rsbuild.config.mjspackages/bruno-app/src/components/ApiSpecPanel/Renderers/Swagger/index.jspackages/bruno-app/src/components/CodeEditor/index.jspackages/bruno-app/src/components/RequestTabPanel/index.jspackages/bruno-app/src/components/RequestTabs/index.jspackages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultPreview/PdfPreview.jspackages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultPreview/index.jspackages/bruno-app/src/components/ResponsePane/QueryResult/index.jspackages/bruno-app/src/components/ResponsePane/ResponseBookmark/index.jspackages/bruno-app/src/components/ResponsePane/ResponseCopy/index.jspackages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.jspackages/bruno-app/src/components/Sidebar/Collections/Collection/index.jspackages/bruno-app/src/components/Sidebar/Sections/CollectionsSection/index.jspackages/bruno-app/src/components/StatusBar/index.jspackages/bruno-app/src/pages/Bruno/index.jspackages/bruno-app/src/pages/Main.jspackages/bruno-app/src/utils/codemirror/setup.jspackages/bruno-app/src/utils/common/format-response.jspackages/bruno-app/src/utils/common/index.js
💤 Files with no reviewable changes (1)
- packages/bruno-app/src/pages/Main.js
| {Array.from(new Array(numPages), (el, index) => ( | ||
| <Page key={`page_${index + 1}`} pageNumber={index + 1} renderAnnotationLayer={false} /> | ||
| ))} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid new Array(null) quirk.
When numPages is null (its initial state), new Array(numPages) evaluates to [null] rather than an empty array. This causes the component to prematurely render one <Page> before the document has loaded, which is logically incorrect.
Use Array.from({ length: numPages || 0 }) to safely generate the array with the expected length.
💚 Proposed fix
- {Array.from(new Array(numPages), (el, index) => (
+ {Array.from({ length: numPages || 0 }, (_, index) => (
<Page key={`page_${index + 1}`} pageNumber={index + 1} renderAnnotationLayer={false} />
))}📝 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.
| {Array.from(new Array(numPages), (el, index) => ( | |
| <Page key={`page_${index + 1}`} pageNumber={index + 1} renderAnnotationLayer={false} /> | |
| ))} | |
| {Array.from({ length: numPages || 0 }, (_, index) => ( | |
| <Page key={`page_${index + 1}`} pageNumber={index + 1} renderAnnotationLayer={false} /> | |
| ))} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/bruno-app/src/components/ResponsePane/QueryResult/QueryResultPreview/PdfPreview.js`
around lines 25 - 27, Update the page iteration in the PDF preview component to
use Array.from with a length of numPages || 0 instead of new Array(numPages),
ensuring the initial null state produces no Page elements while preserving the
existing page numbering and rendering props.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/rsbuild.config.mjs (1)
58-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the module path regex OS-agnostic.
On Windows, the absolute resource paths passed to loaders and parsers use backslashes (
\). This hardcoded forward slash may cause the regex to silently fail on Windows, breaking the lazy-loading fix for Electron. Use[\\/]to safely match both path separators.As per path instructions, ensure all code is OS-agnostic and avoid hardcoding
/or\\separators.💻 Proposed fix
{ - test: /node_modules\/react-player/, + test: /node_modules[\\/]react-player/, parser: { javascript: { dynamicImportMode: 'eager', }, }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/rsbuild.config.mjs` around lines 58 - 65, Update the module path regex in the react-player rule to match both `/` and `\` separators, using the established `[\\/]` pattern so the dynamicImportMode eager configuration applies consistently across operating systems.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/bruno-app/rsbuild.config.mjs`:
- Around line 58-65: Update the module path regex in the react-player rule to
match both `/` and `\` separators, using the established `[\\/]` pattern so the
dynamicImportMode eager configuration applies consistently across operating
systems.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 341ec9a0-d12d-4e96-a38c-36e799d6a1c4
📒 Files selected for processing (6)
packages/bruno-app/rsbuild.config.mjspackages/bruno-app/src/components/Notifications/NotificationsModal/NotificationDetail.jspackages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/ExampleItem/index.jspackages/bruno-app/src/pages/Bruno/index.jspackages/bruno-app/src/utils/codemirror/setup.jspackages/bruno-app/src/utils/terminal.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/pages/Bruno/index.js
- packages/bruno-app/src/utils/codemirror/setup.js
This branch has not been deployed
1 participant
Description
1. Re-enable code splitting
Removed the global
dynamicImportMode: "eager"and addedoutput.assetPrefix: './'so that split chunks load correctly under Electron'sfile://protocol.dynamicImportMode: "eager"is now scoped only toreact-player, which uses internal dynamic imports that fail in Electron without it.2. Move CodeMirror setup out of the main bundle
All CodeMirror modes, addons, themes, and GraphQL plugins were sitting at the top level of
Bruno/index.js, forcing the entire CodeMirror setup to load before first paint. Extracted tosrc/utils/codemirror/setup.jsand imported only fromCodeEditor/index.js, so CodeMirror loads when an editor first mounts — not at startup.3. Lazy-load heavy panels
RequestTabPanel,ApiSpecPanel,ManageWorkspace, andAiChatSidebarare not visible on first paint. They are now loaded asynchronously withReact.lazy()so their code is excluded from the initial bundle.4. Lazy-load sidebar modals
Modal components in
CollectionsSection,Collection, andCollectionItem(import collection, create collection, rename, clone, delete, run, generate code, etc.) are now lazy-loaded. They only mount when the user triggers them.5. Lazy-load
NewRequestinRequestTabsRequestTabsstatically importedNewRequest, which pulledSingleLineEditor→ CodeMirror core andautocomplete.js→@usebruno/common→@faker-js/fakerinto the main bundle.NewRequestis now lazy-loaded since it only renders when the user clicks the new request button.6. Move
prettierout of the main bundleformatResponseand related formatting utilities (which importprettier/standaloneandprettier/parser-babel) were extracted fromutils/common/index.jsinto a separateutils/common/format-response.jsmodule. Since all callers live inside the lazyRequestTabPanelchunk, prettier now only loads when a request tab is open.7. Remove unused CSS from startup
Moved CodeMirror, GraphiQL, and GraphQL docs stylesheets out of
Main.jsandBruno/index.js. These now load with the async chunks that actually need them. Also removed unused Inter font weights (100, 200, 300, 800, 900) — only 400–700 are used in the app.8. Lazy-load PDF viewer
pdfjs-dist(~582 KiB) was imported at the top level ofQueryResultPreview. Extracted into a newPdfPreview.jscomponent that is lazy-loaded only when a PDF response is received.Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Performance
Bug Fixes