feat(web): show busy state on Quick Demo + Refresh buttons during async actions#16
feat(web): show busy state on Quick Demo + Refresh buttons during async actions#16AnkanMisra wants to merge 2 commits intomainfrom
Conversation
…k stuck during the 5-30s 0g anchor: extracted a runWithBusyState(btn, label, handler) helper in main.ts that disables the button, swaps its label for a spinner + the busy text, awaits the handler, and restores the original innerHTML in finally; rewired bindActionButtons to take an ActionConfig (handler + optional busy + busyLabel) so async actions (load-demo, refresh-policies, refresh-timeline) get the loader and sync helpers (presets, modal copy/close) stay zero-overhead; .btn-spinner is now color-adaptive via currentColor + transparent border-top so it reads correctly inside both .btn-primary (dark on lime) and .btn-ghost (light on dark); .btn-ghost gets the same disabled/is-busy opacity + cursor: progress treatment as .btn-primary did from the evaluate panel work
📝 WalkthroughWalkthroughA button action system was refactored to support busy-state UI for async operations. New ChangesButton Busy-State System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Deploying chainshield with
|
| Latest commit: |
84b5d3e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e20f8525.chainshield.pages.dev |
| Branch Preview URL: | https://feat-quick-demo-loading.chainshield.pages.dev |
… the same inline-flex + gap shape as .btn-primary; the screenshot shared by user showed the spinner glyph touching the leading L of LOADING DEMO because .btn-ghost was display: inline-block (browser default for buttons), so the two spans the busy helper injects (<span class=btn-spinner></span><span>Loading demo</span>) sat flush against each other; matching the .btn-primary 0.5rem gap fixes it for both Quick Demo and the two Refresh buttons
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/src/scripts/main.ts (1)
46-55: ⚡ Quick win
innerHTMLassignments trigger static analysis; consider DOM API for defense-in-depthThe ast-grep warnings on lines 49 and 55 are false positives here —
busyLabelis always a hardcoded literal andoriginalLabelis sourced from static Astro-rendered HTML, so there is no actual XSS path. That said, switching to DOM construction eliminates the noise and guards against future callers passing a dynamicbusyLabel:Optional refactor using DOM APIs
- const originalLabel = btn.innerHTML; + const originalLabel = btn.textContent ?? ""; btn.disabled = true; btn.classList.add("is-busy"); - btn.innerHTML = `<span class="btn-spinner" aria-hidden="true"></span><span>${busyLabel}</span>`; + const spinner = document.createElement("span"); + spinner.className = "btn-spinner"; + spinner.setAttribute("aria-hidden", "true"); + const label = document.createElement("span"); + label.textContent = busyLabel; + btn.replaceChildren(spinner, label); try { await handler(); } finally { btn.classList.remove("is-busy"); btn.disabled = false; - btn.innerHTML = originalLabel; + btn.textContent = originalLabel; }Note: this assumes button labels are plain text (confirmed by the Astro templates — "Quick demo", "Refresh"). If a button ever has rich HTML content,
textContentwould lose it; saveinnerHTMLinstead only for that case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/scripts/main.ts` around lines 46 - 55, The code assigns raw HTML to btn.innerHTML for both the busy state and restoring original content which triggers static-analysis warnings; change to DOM APIs: store the original label using btn.textContent (or innerHTML only if you must preserve rich HTML), then for the busy state create a spinner span element (class "btn-spinner", set aria-hidden="true") and a label span with textContent set to busyLabel, clear btn's children and append the spinner and label elements, and finally in the cleanup use btn.textContent = originalLabel (or restore saved innerHTML if preserving rich content) and remove the "is-busy" class and disabled flag; reference symbols: btn, originalLabel, busyLabel, handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/styles/global.css`:
- Line 280: The border declaration uses the non-lowercase keyword "currentColor"
which breaks the Stylelint rule value-keyword-case; update the declaration in
global.css where "border: 2px solid currentColor;" appears to use the lowercase
form "currentcolor" (i.e., "border: 2px solid currentcolor;") so the rule passes
while preserving the same rendering.
- Around line 299-303: The `.btn-ghost.is-busy`/`.btn-ghost:disabled` rule is
being overridden by the later `.btn-ghost:hover` rule; move the
`.btn-ghost:disabled, .btn-ghost.is-busy { ... }` block so it appears after
`.btn-ghost:hover` and explicitly reset the hover-affected properties (color,
border-color, background, cursor, and opacity) inside that disabled/busy rule to
ensure hover cannot override the busy/disabled styling; reference the selectors
`.btn-ghost:disabled`, `.btn-ghost.is-busy`, and `.btn-ghost:hover` when making
the change.
---
Nitpick comments:
In `@web/src/scripts/main.ts`:
- Around line 46-55: The code assigns raw HTML to btn.innerHTML for both the
busy state and restoring original content which triggers static-analysis
warnings; change to DOM APIs: store the original label using btn.textContent (or
innerHTML only if you must preserve rich HTML), then for the busy state create a
spinner span element (class "btn-spinner", set aria-hidden="true") and a label
span with textContent set to busyLabel, clear btn's children and append the
spinner and label elements, and finally in the cleanup use btn.textContent =
originalLabel (or restore saved innerHTML if preserving rich content) and remove
the "is-busy" class and disabled flag; reference symbols: btn, originalLabel,
busyLabel, handler.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 183f3724-2330-4ace-9595-d090910fa489
📒 Files selected for processing (2)
web/src/scripts/main.tsweb/src/styles/global.css
| border-radius: 50%; | ||
| border: 2px solid rgba(12, 13, 12, 0.25); | ||
| border-top-color: var(--ink-bg); | ||
| border: 2px solid currentColor; |
There was a problem hiding this comment.
Fix currentColor casing to satisfy Stylelint value-keyword-case
CSS is case-insensitive with respect to currentColor, so browsers render it correctly regardless of casing, but the project's Stylelint config (value-keyword-case) requires the all-lowercase form and will error here.
Proposed fix
- border: 2px solid currentColor;
+ border: 2px solid currentcolor;📝 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.
| border: 2px solid currentColor; | |
| border: 2px solid currentcolor; |
🧰 Tools
🪛 Stylelint (17.9.0)
[error] 280-280: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/styles/global.css` at line 280, The border declaration uses the
non-lowercase keyword "currentColor" which breaks the Stylelint rule
value-keyword-case; update the declaration in global.css where "border: 2px
solid currentColor;" appears to use the lowercase form "currentcolor" (i.e.,
"border: 2px solid currentcolor;") so the rule passes while preserving the same
rendering.
| .btn-ghost:disabled, | ||
| .btn-ghost.is-busy { | ||
| opacity: 0.7; | ||
| cursor: progress; | ||
| } |
There was a problem hiding this comment.
.btn-ghost.is-busy:hover overrides the busy state — rule must come after :hover
.btn-ghost:hover at line 325 has identical specificity (0,2,0) and appears later in the cascade, so it wins. Hovering a busy ghost button will apply the hover color, border-color, and background, making it look interactive and undoing the visual "disabled" feedback. The .btn-primary counterpart avoids this by placing its disabled rule after its hover rule (lines 259–265 vs 255–258).
The fix requires two things: move the block after .btn-ghost:hover, and explicitly reset the properties that hover changes (since just ordering is not enough — the disabled rule must actively reset them):
Proposed fix
-.btn-ghost:disabled,
-.btn-ghost.is-busy {
- opacity: 0.7;
- cursor: progress;
-}
.btn-ghost {
font-family: var(--mono);
...
}
.btn-ghost:hover {
color: var(--paper);
border-color: var(--paper-soft);
background: var(--ink-2);
}
+/* Must follow :hover so these values win in the cascade */
+.btn-ghost:disabled,
+.btn-ghost.is-busy {
+ opacity: 0.7;
+ cursor: progress;
+ color: var(--paper-soft);
+ border-color: var(--rule);
+ background: transparent;
+}📝 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.
| .btn-ghost:disabled, | |
| .btn-ghost.is-busy { | |
| opacity: 0.7; | |
| cursor: progress; | |
| } | |
| .btn-ghost:disabled, | |
| .btn-ghost.is-busy { | |
| opacity: 0.7; | |
| cursor: progress; | |
| color: var(--paper-soft); | |
| border-color: var(--rule); | |
| background: transparent; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/styles/global.css` around lines 299 - 303, The
`.btn-ghost.is-busy`/`.btn-ghost:disabled` rule is being overridden by the later
`.btn-ghost:hover` rule; move the `.btn-ghost:disabled, .btn-ghost.is-busy { ...
}` block so it appears after `.btn-ghost:hover` and explicitly reset the
hover-affected properties (color, border-color, background, cursor, and opacity)
inside that disabled/busy rule to ensure hover cannot override the busy/disabled
styling; reference the selectors `.btn-ghost:disabled`, `.btn-ghost.is-busy`,
and `.btn-ghost:hover` when making the change.
|
Superseded by #19 which takes a fundamentally better approach: the underlying problem is the 5-30s 0G storage upload, not the lack of a button spinner. PR #19 fires the anchor in the background on the server (response time drops from 5-30s to ~50ms), shows an instant skeleton card in the empty space under 'Active Policies' instead of a button spinner, and polls for the anchor to fill in the pill once it lands. Net result is the user never sees a 10s hang. Keeping this PR open would just dilute the review queue with a partial fix that #19 makes obsolete. |
1 participant
Why
User reported: clicking Quick demo in the deployed UI hangs for 5-6 seconds with no feedback while the server anchors the demo policy on 0G Storage. Looks like the button is stuck. Same issue applies to the Refresh buttons (policies + timeline).
What changes
web/src/scripts/main.ts:runWithBusyState(btn, label, handler)helper that disables the button, swaps its label for<span class="btn-spinner"></span><span>{label}</span>, awaits the async handler, and restores the originalinnerHTMLinfinally.bindActionButtonsto take anActionConfigobject per action:{ handler, busy?, busyLabel? }. Async actions (load-demo,refresh-policies,refresh-timeline) opt into the busy state. Sync helpers (presets, modal copy/close) stay zero-overhead.web/src/styles/global.css:.btn-spinneris now color-adaptive viacurrentColor+border-top-color: transparent. Reads correctly inside both.btn-primary(dark spinner on lime button) and.btn-ghost(light spinner on dark button) without per-variant CSS..btn-ghost:disabled, .btn-ghost.is-busyget the sameopacity: 0.7; cursor: progresstreatment that.btn-primaryalready had from the evaluate-panel work.What this does NOT cover
docs/deploy.md).Test plan
bun test— 93 specs across 11 files, all greenbun run typecheck— server (tsc --noEmit) + web (astro check: 0/0/0) cleanbun run build:web— Astro production build succeedsOut of scope
fetchisn't aborted; not necessary at this latency)Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
Style