Skip to content

fix: Use inline themed SVG for loader spinner#2379

Merged
deadlyjack merged 2 commits into
mainfrom
ajit/fix-ui2
Jun 24, 2026
Merged

fix: Use inline themed SVG for loader spinner#2379
deadlyjack merged 2 commits into
mainfrom
ajit/fix-ui2

Conversation

@deadlyjack

Copy link
Copy Markdown
Member

Import tail-spin.svg as raw SVG markup and render it through the loader DOM instead of using the circular border/background spinner. Update the SVG to inherit currentColor, expand its viewBox to avoid edge clipping, and style the dialog spinner with the app primary theme color.

Add ?raw SVG handling to both Rspack and Webpack so inline SVG imports do not conflict with normal asset/resource SVG usage. Also removes the old theme-side tail-spin recoloring path now that the spinner color is driven by CSS.

Import tail-spin.svg as raw SVG markup and render it through the loader DOM instead of using the circular border/background spinner. Update the SVG to inherit currentColor, expand its viewBox to avoid edge clipping, and style the dialog spinner with the app primary theme color.

Add ?raw SVG handling to both Rspack and Webpack so inline SVG imports do not conflict with normal asset/resource SVG usage. Also removes the old theme-side tail-spin recoloring path now that the spinner color is driven by CSS.
@greptile-apps

greptile-apps Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR replaces the old background-image spinner (a pre-recolored file written to DATA_STORAGE) with an inline SVG that uses currentColor for theming, and adds ?raw query handling to both the Rspack and Webpack configs so SVG imports don't conflict with normal asset/resource usage.

  • Removes the file-system based SVG recoloring path in src/theme/list.js and replaces it with CSS color inheritance through the inlined SVG's currentColor stops.
  • Introduces createTailSpinSvg() in loader.js to stamp unique gradient IDs on each SVG instance, and adds a createTitleLoader() helper that reuses the same DOM element across repeated showTitleLoader calls.
  • Updates showFileInfo.js to use the proper loader.showTitleLoader() / loader.removeTitleLoader() API instead of directly toggling the title-loading class.

Confidence Score: 5/5

The change is safe to merge — it replaces a fragile file-system recoloring path with a straightforward inline SVG approach driven entirely by CSS, with no user-controlled data flowing into the new innerHTML assignments.

The core logic — unique gradient ID stamping via createTailSpinSvg(), singleton title loader DOM reuse, and mutually exclusive ?raw/asset webpack rules — is all implemented correctly. The removal of the DATA_STORAGE write path in list.js simplifies the theme application cycle without dropping any required behavior. No correctness issues were found in the changed code.

No files require special attention.

Important Files Changed

Filename Overview
src/dialogs/loader.js Adds inline SVG spinner via createTailSpinSvg() with per-instance gradient ID stamping, and createTitleLoader() for singleton title-bar spinner DOM management. Logic is correct.
src/theme/list.js Removes the file-system SVG recoloring path (read/write to DATA_STORAGE) and unused fsOperation/Url imports. Also fixes import paths from relative to alias form. Clean removal.
rspack.config.js Adds ?raw SVG rule (asset/source) before the existing asset/resource rule, and guards the latter with resourceQuery: { not: [/raw/] }. Rule ordering and mutual exclusivity are correct.
webpack.config.js Same ?raw SVG handling added as in Rspack config, with consistent rule ordering. Also reformats existing rules (whitespace only). Pre-existing duplicate 'webp' in the asset regex is unchanged.
src/main.scss Replaces the ::after pseudo-element background-image approach with a real #__title-loader element styled via CSS class hierarchy. Adds pointer-events:none and uses var(--popup-text-color) for the spinner color.
src/dialogs/style.scss Replaces circular-loader mixin with inline SVG sizing styles and sets color: var(--primary-color) for the dialog spinner. Removes @use of mixins.scss (no longer needed).
src/res/tail-spin.svg Updates gradient id to tail-spin-gradient (stamped uniquely at runtime), switches stop-color and circle fill from #fff to currentColor, and expands viewBox from 0 0 38 38 to -1 -1 40 40 to prevent stroke clipping.
src/lib/showFileInfo.js Replaces direct class manipulation (app.classList.add/remove) with the proper loader API calls (showTitleLoader/removeTitleLoader). Correct change.
src/cm/themes/noctisLilac.js Adds alpha suffix to activeLine color (#e1def355), making it semi-transparent. Unrelated to the spinner change but clean.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller as showFileInfo / other
    participant Loader as loader.js
    participant DOM as DOM / app
    participant CSS as CSS (main.scss)

    Caller->>Loader: showTitleLoader()
    Loader->>Loader: "createTitleLoader()<br/>get or create #__title-loader span<br/>with uniquified SVG gradient ID"
    Loader->>DOM: "app.append(#__title-loader) if not connected"
    Loader->>DOM: "classList.remove(title-loading-hide)<br/>classList.add(title-loading)"
    DOM->>CSS: ".title-loading #__title-loader display:flex, appear animation"

    Caller->>Loader: removeTitleLoader()
    Loader->>DOM: classList.add(title-loading-hide)
    DOM->>CSS: ".title-loading.title-loading-hide #__title-loader opacity:0, hide animation"
    Note over DOM,CSS: #__title-loader stays in DOM,<br/>pointer-events:none prevents interaction

    Caller->>Loader: loader.create(title, message)
    Loader->>Loader: "createTailSpinSvg()<br/>increment tailSpinSvgId unique gradient ID"
    Loader->>DOM: "append #__loader dialog with inlined SVG"
    Note over Loader,DOM: SVG color driven by<br/>CSS color:var(--primary-color)
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller as showFileInfo / other
    participant Loader as loader.js
    participant DOM as DOM / app
    participant CSS as CSS (main.scss)

    Caller->>Loader: showTitleLoader()
    Loader->>Loader: "createTitleLoader()<br/>get or create #__title-loader span<br/>with uniquified SVG gradient ID"
    Loader->>DOM: "app.append(#__title-loader) if not connected"
    Loader->>DOM: "classList.remove(title-loading-hide)<br/>classList.add(title-loading)"
    DOM->>CSS: ".title-loading #__title-loader display:flex, appear animation"

    Caller->>Loader: removeTitleLoader()
    Loader->>DOM: classList.add(title-loading-hide)
    DOM->>CSS: ".title-loading.title-loading-hide #__title-loader opacity:0, hide animation"
    Note over DOM,CSS: #__title-loader stays in DOM,<br/>pointer-events:none prevents interaction

    Caller->>Loader: loader.create(title, message)
    Loader->>Loader: "createTailSpinSvg()<br/>increment tailSpinSvgId unique gradient ID"
    Loader->>DOM: "append #__loader dialog with inlined SVG"
    Note over Loader,DOM: SVG color driven by<br/>CSS color:var(--primary-color)
Loading

Reviews (2): Last reviewed commit: "fix: duplicate gradient id in spinner.sv..." | Re-trigger Greptile

Comment thread src/res/tail-spin.svg
Comment on lines +4 to 8
<linearGradient x1="8.042%" y1="0%" x2="65.682%" y2="23.865%" id="tail-spin-gradient">
<stop stop-color="currentColor" stop-opacity="0" offset="0%"/>
<stop stop-color="currentColor" stop-opacity=".631" offset="63.146%"/>
<stop stop-color="currentColor" offset="100%"/>
</linearGradient>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Duplicate gradient ID when both spinners are shown simultaneously

The linearGradient carries id="tail-spin-gradient". When the title loader and the dialog loader are both visible (e.g., showTitleLoader is called while a dialog loader is already open), two copies of this SVG are inlined in the document, producing two elements with the same ID. Browsers resolve stroke="url(#tail-spin-gradient)" against the first matching element in the document, so the second spinner's path references the first SVG's gradient. While the gradients are identical and both spinners render correctly in practice, the duplicate ID is technically invalid and can generate console warnings in strict parsers. Scoping the ID (e.g., a unique suffix per instance) or using an inline gradientUnits="userSpaceOnUse" approach without an ID reference would eliminate the ambiguity.

Comment thread src/main.scss
@deadlyjack

Copy link
Copy Markdown
Member Author

@greptile_apps review again

@deadlyjack deadlyjack merged commit 10b6292 into main Jun 24, 2026
10 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in The Code Board - Acode Jun 24, 2026
@deadlyjack deadlyjack deleted the ajit/fix-ui2 branch June 24, 2026 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant