Skip to content

#435: show spinner indicating page is loading - #449

Draft
djyotta wants to merge 16 commits into
sqlpage:mainfrom
djyotta:daniel/435-spinner-to-indicate-page-is-loading
Draft

djyotta wants to merge 16 commits into
sqlpage:mainfrom
djyotta:daniel/435-spinner-to-indicate-page-is-loading

Conversation

@djyotta

@djyotta djyotta commented Jun 23, 2024 •

Copy link
Copy Markdown
Contributor

Closes #435

@djyotta
djyotta marked this pull request as draft June 23, 2024 21:42
@djyotta

djyotta commented Jun 23, 2024

Copy link
Copy Markdown
Contributor Author

@lovasoa can you take a look to see if it's ok in principle before I start on documentation?

@djyotta
djyotta marked this pull request as ready for review June 26, 2024 05:24
The name of a [spinner](https://tabler.io/docs/components/spinners) (from tabler.io).
Default is "spinner-border".
Set to the empty string to disable the spinner - e.g. to display only progress
updates.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've noticed this whole approach doesn't work inside run_sql for some reason. So I should note that 'loader-start' component should be selected before descending into run_sql.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, run_sql is a function, it returns a single json value all at once, it can't stream its results.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought as much but didn't want to assume. It's good to have the clarification.

@lovasoa
lovasoa force-pushed the daniel/435-spinner-to-indicate-page-is-loading branch from 1c3024a to 0bd66fb Compare September 27, 2026 06:49
Comment on lines +1 to +6
<div {{#if id}}id="{{id}}"{{/if}} class="sqlpage-loader-start">
<div class="sqlpage-loader-container">
<span class="{{default spinner "spinner-border"}}
{{#if size}}spinner-border-{{size}}{{/if}}
{{#if color}}text-{{color}}{{/if}}">
</span>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

errors between loader-start and loader-stop show up in the spinner's box

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I knew about this one. I didn't even begin to think about how to solve that... lol, just make sure it doesn't error.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think you can fix it by closing the <div>. something like

<div {{#if id}}id="{{id}}"{{/if}} class="sqlpage-loader" role="status">
  <span class="{{default spinner "spinner-border"}} {{#if color}}text-{{color}}{{/if}}"></span>
</div>

@djyotta djyotta Oct 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could be forgetting, but I thought the whole hack here was leaving the div open so the spinner disappears when it's closed... My CSS evolved over time though, and it looks as though closing the div should work.

I just recall leaving it open was important for some reason.

It's closed in the sqlpage-loader-stop element.

Comment on lines +6 to +10
{{~#if (not percent)}} progress-bar-indeterminate{{/if}}
{{~#if color}} bg-{{color}}{{/if}}"
role="progressbar"
aria-valuenow="{{percent}}"
{{~#if percent}}style="width: {{percent}}%; display: block"{{/if}}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

SELECT 'progress' AS component, 0 AS percent renders progress-bar-indeterminate because of an improper null check

Comment on lines +15 to +18
{{~#if stage}}
<br>
<label for="sqlpage-loading-{{default stage "progress"}}">{{stage}}</label>
{{/if}}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment on lines +1 to +4
-- Disable actix-web compression middleware:
-- https://github.com/actix/actix-web/issues/3410
SELECT 'http_header' AS component, 'Identity' AS "Content-Encoding";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we need this still? actix/actix-web#341 is closed

@djyotta djyotta Oct 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's not immediately obvious to me how actix/actix-web#341 is related.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ope, sorry missed a 0 at the end

actix/actix-web#3410

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I implemented this not as a work around for some buffering issue, but because I have some legitimately long running queries so the spinner helps indicate something is happening.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

sure, but its fixed upstream now. I think the better approach would be to update the actix-web dependency and use the upstream fix :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I misunderstood your comment at the first. My bad.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nw, thanks for the PR, excited to see the next revision :)
please re-request review when ready

@81reap 81reap self-assigned this Oct 30, 2026
@lovasoa
lovasoa marked this pull request as draft October 30, 2026 20:55
@lovasoa

lovasoa commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator

I switched the pr to draft, @djyotta feel free to switch it back when ready

@datadog-official

datadog-official Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Pipelines

❌ Errors

Your PR has failed checks. Please review the issues below and take necessary action before merging.

🚦 2 Pipeline jobs failed

CI | CI gate — 🔧 Needs a code fix, caused by this PR

View more details · View in GitHub Actions

Compilation and linting errors occurred, causing the job to fail with exit code 1.

CI | compile_and_lint — 🔧 Needs a code fix, caused by this PR

View more details · View in GitHub Actions

TypeScript compilation error in test.ts: TS2835: Relative import paths need explicit file extensions when '--moduleResolution' is 'node16'. Did you mean '../../fixture.js'?

📋 Copy fix prompt
CI on my pull request is failing. Help me find and fix the root cause of each failing job below — they were flagged as caused by changes in this PR, so focus on the diff. For each job, explain the failure and propose a fix.

Before you start, set up the Datadog software-delivery tooling so you can
query the CI data yourself:

1. Check whether you already have the Datadog software-delivery MCP tools
   (e.g. a `search_datadog_ci_pipeline_events` tool) and the `unblock-pr` skill.
2. If either is missing, STOP and ask me for permission before installing
   anything. Do not install or run anything until I have said yes.
3. Only with my explicit approval, set up the Datadog software-delivery MCP
   server and skills by following:
     https://docs.datadoghq.com/getting_started/software_delivery_mcp_tools/
   then restart so the skill is picked up.
4. If I decline, skip all of the above and work from the context below alone.

Then run /unblock-pr — it will pull the CI data itself. The job context below is what we already know.

If /unblock-pr is not available — because I declined the setup above, or it did not install — work from the context below instead.

Datadog has already classified this failure as caused by changes in this PR.
Take that as given and work the fix:

1. Locate the change. Diff this branch against its base and find the change
   that produces this error. Explain the mechanism, don't just name a file:
     git fetch origin && git diff $(git merge-base origin/main HEAD)...HEAD
2. Reproduce it locally. Run the failing job's command or test before
   proposing anything.
3. Propose the smallest fix that addresses the root cause — not a workaround,
   not a broadened assertion, not a disabled or skipped test.
4. Re-run the same command to confirm, and say exactly what you ran.
5. If the failure turns out to be intermittent rather than deterministic, say
   so plainly instead of "fixing" it — that is a flaky test, and patching it
   hides the problem.

If the right move is to re-run the job rather than change code, use the job
link in the context below. For GitHub Actions: `gh run rerun <run-id> --failed`,
where the run ID is the number after `/runs/` in that URL (not the trailing
number, which is the job ID).

Branch: daniel/435-spinner-to-indicate-page-is-loading

CI | CI gate
Commit: 5045a4f322b89c850a52e97306549a2c22bea787
Error (code / build):
Compilation and linting errors occurred, causing the job to fail with exit code 1.
CI job: https://github.com/sqlpage/SQLPage/actions/runs/41126977264/job/124313807413

CI | compile_and_lint
Commit: 5045a4f322b89c850a52e97306549a2c22bea787
Error (code / build):
TypeScript compilation error in test.ts: TS2835: Relative import paths need explicit file extensions when '--moduleResolution' is 'node16'. Did you mean '../../fixture.js'?
CI job: https://github.com/sqlpage/SQLPage/actions/runs/41126977264/job/124307959377

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 5045a4f | Docs | View more details | Give us feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Spinner to indicate page is loading

3 participants