fix: added loading state for initial load#7486
Open
cchirag wants to merge 1 commit intousebruno:mainfrom
Open
Conversation
cchirag
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
WalkthroughThe Spinner component is refactored with configurable sizes and CSS keyframe animations, and RequestTabPanel now displays a loading state when the active workspace lacks scratchCollectionUid. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
![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: 2
🧹 Nitpick comments (1)
packages/bruno-app/src/components/Spinner/StyledWrapper.js (1)
54-57: Respect reduced-motion for the infinite spinner animation.This now animates forever for every spinner. Please disable it under
prefers-reduced-motionso motion-sensitive users get a static indicator instead.Possible fix
.spinner { display: inline-flex; animation: ${spin} 0.75s linear infinite; } + + `@media` (prefers-reduced-motion: reduce) { + .spinner { + animation: none; + } + } .spinner-text { font-size: 0.875rem; color: inherit;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/Spinner/StyledWrapper.js` around lines 54 - 57, The spinner CSS currently always applies animation: ${spin} 0.75s linear infinite on the .spinner selector; add a prefers-reduced-motion media query that targets .spinner and disables the animation (e.g., set animation: none) so motion-sensitive users see a static indicator. Update the StyledWrapper where .spinner and the spin keyframe are defined to include `@media` (prefers-reduced-motion: reduce) { .spinner { animation: none; } }.
🤖 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/RequestTabPanel/index.js`:
- Around line 180-186: RequestTabPanel is using the absence of
activeWorkspace?.scratchCollectionUid to show a loading spinner which can
permanently block rendering when mountScratchCollection() returns null (and
switchWorkspace() continues); change the guard so that rendering is keyed off an
explicit workspace-loading flag (e.g., workspaceLoading from the workspace slice
or the action that calls mountScratchCollection/switchWorkspace) or move the
scratchCollectionUid check below the branches that render tabs which do not
depend on the scratch collection (for example the "preferences",
"global-environment-settings", and workspace tabs). Specifically, in
RequestTabPanel replace the current top-level check on
activeWorkspace?.scratchCollectionUid with either (a) a check for a dedicated
loading boolean provided by the workspaces reducer/actions (consume
workspaceLoading) before showing the spinner, or (b) relocate the
scratchCollectionUid-dependent guard to only wrap components that need it,
leaving tab branches that don't depend on scratchCollectionUid to render
normally; reference mountScratchCollection, switchWorkspace, activeWorkspace,
scratchCollectionUid and the RequestTabPanel component to locate the code to
change.
In `@packages/bruno-app/src/components/Spinner/index.js`:
- Around line 4-35: The Spinner component currently renders an unlabeled SVG;
update the component (Spinner, StyledWrapper and the span.spinner wrapper) to
include role="status" on the wrapper and provide an accessible label when no
children are passed (e.g., set aria-label="Loading" or aria-live if you prefer)
so assistive tech announces the loading state, and mark the SVG as purely
decorative by adding aria-hidden="true" (and focusable="false") on the svg
element; ensure existing visible children still render as the announcement
instead of the default label.
---
Nitpick comments:
In `@packages/bruno-app/src/components/Spinner/StyledWrapper.js`:
- Around line 54-57: The spinner CSS currently always applies animation: ${spin}
0.75s linear infinite on the .spinner selector; add a prefers-reduced-motion
media query that targets .spinner and disables the animation (e.g., set
animation: none) so motion-sensitive users see a static indicator. Update the
StyledWrapper where .spinner and the spin keyframe are defined to include `@media`
(prefers-reduced-motion: reduce) { .spinner { animation: none; } }.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 473681db-d16e-44cb-8d0f-eab14b489258
📒 Files selected for processing (3)
packages/bruno-app/src/components/RequestTabPanel/index.jspackages/bruno-app/src/components/Spinner/StyledWrapper.jspackages/bruno-app/src/components/Spinner/index.js
Comment on lines
+180
to
+186
| if (!activeWorkspace?.scratchCollectionUid) { | ||
| return ( | ||
| <div className="w-full h-full flex items-center justify-center"> | ||
| <Spinner size="lg" /> | ||
| </div> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't use scratchCollectionUid absence as the loading signal.
mountScratchCollection() can return null in packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js:986-1049, and switchWorkspace() still continues in packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js:347-398. In that path, Line 180 becomes a permanent spinner, and it also blocks tabs like preferences, global-environment-settings, and workspace tabs that do not depend on the scratch collection at all. Please key this off an explicit workspace-loading flag, or move the guard below the branches that can render without scratchCollectionUid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/RequestTabPanel/index.js` around lines 180
- 186, RequestTabPanel is using the absence of
activeWorkspace?.scratchCollectionUid to show a loading spinner which can
permanently block rendering when mountScratchCollection() returns null (and
switchWorkspace() continues); change the guard so that rendering is keyed off an
explicit workspace-loading flag (e.g., workspaceLoading from the workspace slice
or the action that calls mountScratchCollection/switchWorkspace) or move the
scratchCollectionUid check below the branches that render tabs which do not
depend on the scratch collection (for example the "preferences",
"global-environment-settings", and workspace tabs). Specifically, in
RequestTabPanel replace the current top-level check on
activeWorkspace?.scratchCollectionUid with either (a) a check for a dedicated
loading boolean provided by the workspaces reducer/actions (consume
workspaceLoading) before showing the spinner, or (b) relocate the
scratchCollectionUid-dependent guard to only wrap components that need it,
leaving tab branches that don't depend on scratchCollectionUid to render
normally; reference mountScratchCollection, switchWorkspace, activeWorkspace,
scratchCollectionUid and the RequestTabPanel component to locate the code to
change.
Comment on lines
+4
to
+35
| const Spinner = ({ size = 'md', children }) => { | ||
| const sizeMap = { | ||
| xs: 12, | ||
| sm: 16, | ||
| md: 20, | ||
| lg: 24, | ||
| xl: 32 | ||
| }; | ||
|
|
||
| const spinnerSize = sizeMap[size] || 20; | ||
|
|
||
| return ( | ||
| <StyledWrapper> | ||
| <div className="animate-spin"></div> | ||
| {children && <div>{children}</div>} | ||
| <StyledWrapper $size={size}> | ||
| <span className="spinner"> | ||
| <svg | ||
| width={spinnerSize} | ||
| height={spinnerSize} | ||
| viewBox="0 0 24 24" | ||
| fill="none" | ||
| > | ||
| <circle | ||
| cx="12" | ||
| cy="12" | ||
| r="10" | ||
| stroke="currentColor" | ||
| strokeWidth="3" | ||
| strokeLinecap="round" | ||
| strokeDasharray="31.4 31.4" | ||
| /> | ||
| </svg> | ||
| </span> | ||
| {children && <div className="spinner-text">{children}</div>} |
Contributor
There was a problem hiding this comment.
Expose status semantics for standalone spinners.
The new initial-load path in packages/bruno-app/src/components/RequestTabPanel/index.js:180-184 renders this with no text. Right now it's just an unlabeled SVG, so assistive tech gets no loading announcement. Please give the wrapper a status role/label and mark the SVG decorative.
Possible fix
-const Spinner = ({ size = 'md', children }) => {
+const Spinner = ({ size = 'md', children, label = 'Loading' }) => {
const sizeMap = {
xs: 12,
sm: 16,
md: 20,
lg: 24,
xl: 32
};
const spinnerSize = sizeMap[size] || 20;
return (
- <StyledWrapper $size={size}>
- <span className="spinner">
+ <StyledWrapper
+ $size={size}
+ role="status"
+ aria-label={children ? undefined : label}
+ >
+ <span className="spinner" aria-hidden="true">
<svg
width={spinnerSize}
height={spinnerSize}
viewBox="0 0 24 24"
fill="none"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const Spinner = ({ size = 'md', children }) => { | |
| const sizeMap = { | |
| xs: 12, | |
| sm: 16, | |
| md: 20, | |
| lg: 24, | |
| xl: 32 | |
| }; | |
| const spinnerSize = sizeMap[size] || 20; | |
| return ( | |
| <StyledWrapper> | |
| <div className="animate-spin"></div> | |
| {children && <div>{children}</div>} | |
| <StyledWrapper $size={size}> | |
| <span className="spinner"> | |
| <svg | |
| width={spinnerSize} | |
| height={spinnerSize} | |
| viewBox="0 0 24 24" | |
| fill="none" | |
| > | |
| <circle | |
| cx="12" | |
| cy="12" | |
| r="10" | |
| stroke="currentColor" | |
| strokeWidth="3" | |
| strokeLinecap="round" | |
| strokeDasharray="31.4 31.4" | |
| /> | |
| </svg> | |
| </span> | |
| {children && <div className="spinner-text">{children}</div>} | |
| const Spinner = ({ size = 'md', children, label = 'Loading' }) => { | |
| const sizeMap = { | |
| xs: 12, | |
| sm: 16, | |
| md: 20, | |
| lg: 24, | |
| xl: 32 | |
| }; | |
| const spinnerSize = sizeMap[size] || 20; | |
| return ( | |
| <StyledWrapper | |
| $size={size} | |
| role="status" | |
| aria-label={children ? undefined : label} | |
| > | |
| <span className="spinner" aria-hidden="true"> | |
| <svg | |
| width={spinnerSize} | |
| height={spinnerSize} | |
| viewBox="0 0 24 24" | |
| fill="none" | |
| > | |
| <circle | |
| cx="12" | |
| cy="12" | |
| r="10" | |
| stroke="currentColor" | |
| strokeWidth="3" | |
| strokeLinecap="round" | |
| strokeDasharray="31.4 31.4" | |
| /> | |
| </svg> | |
| </span> | |
| {children && <div className="spinner-text">{children}</div>} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/Spinner/index.js` around lines 4 - 35, The
Spinner component currently renders an unlabeled SVG; update the component
(Spinner, StyledWrapper and the span.spinner wrapper) to include role="status"
on the wrapper and provide an accessible label when no children are passed
(e.g., set aria-label="Loading" or aria-live if you prefer) so assistive tech
announces the loading state, and mark the SVG as purely decorative by adding
aria-hidden="true" (and focusable="false") on the svg element; ensure existing
visible children still render as the announcement instead of the default label.
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
Release Notes
New Features
Style