Open
Conversation
Contributor
Greptile SummaryThis PR fixes a flash of skeleton UI during SSR by (1) calling
Confidence Score: 4/5Safe to merge for the skeleton fix, but the reloading opacity regression should be addressed before landing. The vendor/ change is correct and focused. The SignedIn.svelte change achieves its goal (no skeleton flash on SSR) but introduces a P1 UX regression: filter-change reloads no longer show any visual feedback for the majority of users who arrive via SSR. app/javascript/pages/Home/SignedIn.svelte — the reloading overlay logic needs to be preserved or re-implemented for the SSR render path. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Page Request] --> B{SSR?}
B -- Yes --> C[App.svelte: setPage sync]
C --> D{dashboard_stats in props?}
B -- No / Client Nav --> D
D -- Yes --> E[Render dashboard content directly\nno skeleton, no reloading overlay]
D -- No --> F[Render Deferred component]
F --> G{Data loaded?}
G -- No --> H[Show Skeleton fallback]
G -- Yes --> I[Render dashboard content\nwith reloading opacity support]
E --> J[User changes filters]
J --> K[refreshDashboardData via router.visit]
K --> L[dashboard_stats updated\nbut NO loading indicator shown]
I --> J
Prompt To Fix All With AIThis is a comment left during a code review.
Path: app/javascript/pages/Home/SignedIn.svelte
Line: 136-209
Comment:
**Reloading opacity lost in SSR branch**
When `dashboard_stats` is already populated on SSR/hydration the component permanently stays in the `{#if dashboard_stats}` branch. That branch has no `reloading` indicator, so when `refreshDashboardData` triggers a partial Inertia visit (`only: ["dashboard_stats"]`), the old data stays visible with zero visual feedback — the `class:opacity-60={reloading}` that was in the `<Deferred>` children snippet is effectively unreachable for any user who arrives via SSR (which is the common path after this change).
Consider forwarding the `reloading` state from the `<Deferred>` snippet into a shared variable, or wrapping the SSR branch in the same `<Deferred>` and skipping the fallback skeleton conditionally.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: app/javascript/pages/Home/SignedIn.svelte
Line: 136-166
Comment:
**Duplicated dashboard rendering logic**
The `TodaySentence`, `Dashboard`, and `ActivityGraph` rendering is now copy-pasted verbatim between lines 137–166 (SSR branch) and lines 179–207 (Deferred children snippet). Per the project style guide, duplicated markup like this should be extracted into a shared Svelte snippet or child component. A simple `{#snippet dashboardContent(reloading)}` declared once above the `{#if}` block would eliminate both copies and make the `reloading` opacity easy to restore.
**Context Used:** In the Hackatime repo, please ensure that the PR i... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Don't show skeleton on SSR!" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+136
to
+209
| {#if dashboard_stats} | ||
| <div class="flex flex-col gap-8"> | ||
| <!-- Today Stats --> | ||
| <div> | ||
| {#if dashboard_stats.today_stats} | ||
| <TodaySentence | ||
| show_logged_time_sentence={dashboard_stats.today_stats | ||
| .show_logged_time_sentence} | ||
| todays_duration_display={dashboard_stats.today_stats | ||
| .todays_duration_display} | ||
| todays_languages={dashboard_stats.today_stats.todays_languages} | ||
| todays_editors={dashboard_stats.today_stats.todays_editors} | ||
| /> | ||
| {/if} | ||
| </div> | ||
| {/snippet} | ||
|
|
||
| {#snippet children({ reloading })} | ||
| <div class="flex flex-col gap-8" class:opacity-60={reloading}> | ||
| <!-- Today Stats --> | ||
| <div> | ||
| {#if dashboard_stats?.today_stats} | ||
| <TodaySentence | ||
| show_logged_time_sentence={dashboard_stats.today_stats | ||
| .show_logged_time_sentence} | ||
| todays_duration_display={dashboard_stats.today_stats | ||
| .todays_duration_display} | ||
| todays_languages={dashboard_stats.today_stats.todays_languages} | ||
| todays_editors={dashboard_stats.today_stats.todays_editors} | ||
| /> | ||
| {/if} | ||
|
|
||
| <!-- Main Dashboard --> | ||
| {#if dashboard_stats.filterable_dashboard_data} | ||
| <Dashboard | ||
| data={dashboard_stats.filterable_dashboard_data} | ||
| programmingGoalsProgress={dashboard_stats.programming_goals_progress || | ||
| []} | ||
| onFiltersChange={refreshDashboardData} | ||
| /> | ||
| {/if} | ||
|
|
||
| <!-- Activity Graph --> | ||
| {#if dashboard_stats.activity_graph} | ||
| <ActivityGraph data={dashboard_stats.activity_graph} /> | ||
| {/if} | ||
| </div> | ||
| {:else} | ||
| <Deferred data="dashboard_stats"> | ||
| {#snippet fallback()} | ||
| <div class="flex flex-col gap-8"> | ||
| <div> | ||
| <TodaySentenceSkeleton /> | ||
| </div> | ||
| <DashboardSkeleton /> | ||
| <ActivityGraphSkeleton /> | ||
| </div> | ||
| {/snippet} | ||
|
|
||
| <!-- Main Dashboard --> | ||
| {#if dashboard_stats?.filterable_dashboard_data} | ||
| <Dashboard | ||
| data={dashboard_stats.filterable_dashboard_data} | ||
| programmingGoalsProgress={dashboard_stats.programming_goals_progress || | ||
| []} | ||
| onFiltersChange={refreshDashboardData} | ||
| /> | ||
| {/if} | ||
| {#snippet children({ reloading })} | ||
| <div class="flex flex-col gap-8" class:opacity-60={reloading}> | ||
| <div> | ||
| {#if dashboard_stats?.today_stats} | ||
| <TodaySentence | ||
| show_logged_time_sentence={dashboard_stats.today_stats | ||
| .show_logged_time_sentence} | ||
| todays_duration_display={dashboard_stats.today_stats | ||
| .todays_duration_display} | ||
| todays_languages={dashboard_stats.today_stats.todays_languages} | ||
| todays_editors={dashboard_stats.today_stats.todays_editors} | ||
| /> | ||
| {/if} | ||
| </div> | ||
|
|
||
| <!-- Activity Graph --> | ||
| {#if dashboard_stats?.activity_graph} | ||
| <ActivityGraph data={dashboard_stats.activity_graph} /> | ||
| {/if} | ||
| </div> | ||
| {/snippet} | ||
| </Deferred> | ||
| {#if dashboard_stats?.filterable_dashboard_data} | ||
| <Dashboard | ||
| data={dashboard_stats.filterable_dashboard_data} | ||
| programmingGoalsProgress={dashboard_stats | ||
| .programming_goals_progress || []} | ||
| onFiltersChange={refreshDashboardData} | ||
| /> | ||
| {/if} | ||
|
|
||
| {#if dashboard_stats?.activity_graph} | ||
| <ActivityGraph data={dashboard_stats.activity_graph} /> | ||
| {/if} | ||
| </div> | ||
| {/snippet} | ||
| </Deferred> | ||
| {/if} |
Contributor
There was a problem hiding this comment.
When dashboard_stats is already populated on SSR/hydration the component permanently stays in the {#if dashboard_stats} branch. That branch has no reloading indicator, so when refreshDashboardData triggers a partial Inertia visit (only: ["dashboard_stats"]), the old data stays visible with zero visual feedback — the class:opacity-60={reloading} that was in the <Deferred> children snippet is effectively unreachable for any user who arrives via SSR (which is the common path after this change).
Consider forwarding the reloading state from the <Deferred> snippet into a shared variable, or wrapping the SSR branch in the same <Deferred> and skipping the fallback skeleton conditionally.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/javascript/pages/Home/SignedIn.svelte
Line: 136-209
Comment:
**Reloading opacity lost in SSR branch**
When `dashboard_stats` is already populated on SSR/hydration the component permanently stays in the `{#if dashboard_stats}` branch. That branch has no `reloading` indicator, so when `refreshDashboardData` triggers a partial Inertia visit (`only: ["dashboard_stats"]`), the old data stays visible with zero visual feedback — the `class:opacity-60={reloading}` that was in the `<Deferred>` children snippet is effectively unreachable for any user who arrives via SSR (which is the common path after this change).
Consider forwarding the `reloading` state from the `<Deferred>` snippet into a shared variable, or wrapping the SSR branch in the same `<Deferred>` and skipping the fallback skeleton conditionally.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+136
to
+166
| {#if dashboard_stats} | ||
| <div class="flex flex-col gap-8"> | ||
| <!-- Today Stats --> | ||
| <div> | ||
| {#if dashboard_stats.today_stats} | ||
| <TodaySentence | ||
| show_logged_time_sentence={dashboard_stats.today_stats | ||
| .show_logged_time_sentence} | ||
| todays_duration_display={dashboard_stats.today_stats | ||
| .todays_duration_display} | ||
| todays_languages={dashboard_stats.today_stats.todays_languages} | ||
| todays_editors={dashboard_stats.today_stats.todays_editors} | ||
| /> | ||
| {/if} | ||
| </div> | ||
| {/snippet} | ||
|
|
||
| {#snippet children({ reloading })} | ||
| <div class="flex flex-col gap-8" class:opacity-60={reloading}> | ||
| <!-- Today Stats --> | ||
| <div> | ||
| {#if dashboard_stats?.today_stats} | ||
| <TodaySentence | ||
| show_logged_time_sentence={dashboard_stats.today_stats | ||
| .show_logged_time_sentence} | ||
| todays_duration_display={dashboard_stats.today_stats | ||
| .todays_duration_display} | ||
| todays_languages={dashboard_stats.today_stats.todays_languages} | ||
| todays_editors={dashboard_stats.today_stats.todays_editors} | ||
| /> | ||
| {/if} | ||
|
|
||
| <!-- Main Dashboard --> | ||
| {#if dashboard_stats.filterable_dashboard_data} | ||
| <Dashboard | ||
| data={dashboard_stats.filterable_dashboard_data} | ||
| programmingGoalsProgress={dashboard_stats.programming_goals_progress || | ||
| []} | ||
| onFiltersChange={refreshDashboardData} | ||
| /> | ||
| {/if} | ||
|
|
||
| <!-- Activity Graph --> | ||
| {#if dashboard_stats.activity_graph} | ||
| <ActivityGraph data={dashboard_stats.activity_graph} /> | ||
| {/if} | ||
| </div> |
Contributor
There was a problem hiding this comment.
The TodaySentence, Dashboard, and ActivityGraph rendering is now copy-pasted verbatim between lines 137–166 (SSR branch) and lines 179–207 (Deferred children snippet). Per the project style guide, duplicated markup like this should be extracted into a shared Svelte snippet or child component. A simple {#snippet dashboardContent(reloading)} declared once above the {#if} block would eliminate both copies and make the reloading opacity easy to restore.
Context Used: In the Hackatime repo, please ensure that the PR i... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/javascript/pages/Home/SignedIn.svelte
Line: 136-166
Comment:
**Duplicated dashboard rendering logic**
The `TodaySentence`, `Dashboard`, and `ActivityGraph` rendering is now copy-pasted verbatim between lines 137–166 (SSR branch) and lines 179–207 (Deferred children snippet). Per the project style guide, duplicated markup like this should be extracted into a shared Svelte snippet or child component. A simple `{#snippet dashboardContent(reloading)}` declared once above the `{#if}` block would eliminate both copies and make the `reloading` opacity easy to restore.
**Context Used:** In the Hackatime repo, please ensure that the PR i... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent dashboard skeleton UI from rendering during SSR by ensuring deferred content can resolve against real page props, and by adjusting the Home/SignedIn dashboard rendering logic.
Changes:
- Pre-populate Inertia’s global
pagestore before the initial render in the Svelte adapterApp.svelteto support SSR resolution forDeferred/WhenVisible. - Update
Home/SignedIn.svelteto render dashboard content immediately whendashboard_statsis already present, otherwise fall back to<Deferred>with skeletons.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| vendor/inertia/packages/svelte/dist/components/App.svelte | Sets the global page store earlier to affect SSR behavior for deferred content. |
| app/javascript/pages/Home/SignedIn.svelte | Avoids skeleton rendering when dashboard_stats is present by bypassing <Deferred> in that case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+136
to
+167
| {#if dashboard_stats} | ||
| <div class="flex flex-col gap-8"> | ||
| <!-- Today Stats --> | ||
| <div> | ||
| {#if dashboard_stats.today_stats} | ||
| <TodaySentence | ||
| show_logged_time_sentence={dashboard_stats.today_stats | ||
| .show_logged_time_sentence} | ||
| todays_duration_display={dashboard_stats.today_stats | ||
| .todays_duration_display} | ||
| todays_languages={dashboard_stats.today_stats.todays_languages} | ||
| todays_editors={dashboard_stats.today_stats.todays_editors} | ||
| /> | ||
| {/if} | ||
| </div> | ||
| {/snippet} | ||
|
|
||
| {#snippet children({ reloading })} | ||
| <div class="flex flex-col gap-8" class:opacity-60={reloading}> | ||
| <!-- Today Stats --> | ||
| <div> | ||
| {#if dashboard_stats?.today_stats} | ||
| <TodaySentence | ||
| show_logged_time_sentence={dashboard_stats.today_stats | ||
| .show_logged_time_sentence} | ||
| todays_duration_display={dashboard_stats.today_stats | ||
| .todays_duration_display} | ||
| todays_languages={dashboard_stats.today_stats.todays_languages} | ||
| todays_editors={dashboard_stats.today_stats.todays_editors} | ||
| /> | ||
| {/if} | ||
|
|
||
| <!-- Main Dashboard --> | ||
| {#if dashboard_stats.filterable_dashboard_data} | ||
| <Dashboard | ||
| data={dashboard_stats.filterable_dashboard_data} | ||
| programmingGoalsProgress={dashboard_stats.programming_goals_progress || | ||
| []} | ||
| onFiltersChange={refreshDashboardData} | ||
| /> | ||
| {/if} | ||
|
|
||
| <!-- Activity Graph --> | ||
| {#if dashboard_stats.activity_graph} | ||
| <ActivityGraph data={dashboard_stats.activity_graph} /> | ||
| {/if} | ||
| </div> | ||
| {:else} |
There was a problem hiding this comment.
By bypassing <Deferred> entirely when dashboard_stats is already present, the page will no longer get the reloading state / dimming behavior during filter refreshes (router.visit(... preserveState: true ...)). Consider keeping <Deferred> mounted on the client even when data is present (so reloading continues to work) and only skipping it for SSR, or otherwise reintroduce a loading indicator for refreshes.
Comment on lines
+136
to
+206
| {#if dashboard_stats} | ||
| <div class="flex flex-col gap-8"> | ||
| <!-- Today Stats --> | ||
| <div> | ||
| {#if dashboard_stats.today_stats} | ||
| <TodaySentence | ||
| show_logged_time_sentence={dashboard_stats.today_stats | ||
| .show_logged_time_sentence} | ||
| todays_duration_display={dashboard_stats.today_stats | ||
| .todays_duration_display} | ||
| todays_languages={dashboard_stats.today_stats.todays_languages} | ||
| todays_editors={dashboard_stats.today_stats.todays_editors} | ||
| /> | ||
| {/if} | ||
| </div> | ||
| {/snippet} | ||
|
|
||
| {#snippet children({ reloading })} | ||
| <div class="flex flex-col gap-8" class:opacity-60={reloading}> | ||
| <!-- Today Stats --> | ||
| <div> | ||
| {#if dashboard_stats?.today_stats} | ||
| <TodaySentence | ||
| show_logged_time_sentence={dashboard_stats.today_stats | ||
| .show_logged_time_sentence} | ||
| todays_duration_display={dashboard_stats.today_stats | ||
| .todays_duration_display} | ||
| todays_languages={dashboard_stats.today_stats.todays_languages} | ||
| todays_editors={dashboard_stats.today_stats.todays_editors} | ||
| /> | ||
| {/if} | ||
|
|
||
| <!-- Main Dashboard --> | ||
| {#if dashboard_stats.filterable_dashboard_data} | ||
| <Dashboard | ||
| data={dashboard_stats.filterable_dashboard_data} | ||
| programmingGoalsProgress={dashboard_stats.programming_goals_progress || | ||
| []} | ||
| onFiltersChange={refreshDashboardData} | ||
| /> | ||
| {/if} | ||
|
|
||
| <!-- Activity Graph --> | ||
| {#if dashboard_stats.activity_graph} | ||
| <ActivityGraph data={dashboard_stats.activity_graph} /> | ||
| {/if} | ||
| </div> | ||
| {:else} | ||
| <Deferred data="dashboard_stats"> | ||
| {#snippet fallback()} | ||
| <div class="flex flex-col gap-8"> | ||
| <div> | ||
| <TodaySentenceSkeleton /> | ||
| </div> | ||
| <DashboardSkeleton /> | ||
| <ActivityGraphSkeleton /> | ||
| </div> | ||
| {/snippet} | ||
|
|
||
| <!-- Main Dashboard --> | ||
| {#if dashboard_stats?.filterable_dashboard_data} | ||
| <Dashboard | ||
| data={dashboard_stats.filterable_dashboard_data} | ||
| programmingGoalsProgress={dashboard_stats.programming_goals_progress || | ||
| []} | ||
| onFiltersChange={refreshDashboardData} | ||
| /> | ||
| {/if} | ||
| {#snippet children({ reloading })} | ||
| <div class="flex flex-col gap-8" class:opacity-60={reloading}> | ||
| <div> | ||
| {#if dashboard_stats?.today_stats} | ||
| <TodaySentence | ||
| show_logged_time_sentence={dashboard_stats.today_stats | ||
| .show_logged_time_sentence} | ||
| todays_duration_display={dashboard_stats.today_stats | ||
| .todays_duration_display} | ||
| todays_languages={dashboard_stats.today_stats.todays_languages} | ||
| todays_editors={dashboard_stats.today_stats.todays_editors} | ||
| /> | ||
| {/if} | ||
| </div> | ||
|
|
||
| <!-- Activity Graph --> | ||
| {#if dashboard_stats?.activity_graph} | ||
| <ActivityGraph data={dashboard_stats.activity_graph} /> | ||
| {/if} | ||
| </div> | ||
| {/snippet} | ||
| </Deferred> | ||
| {#if dashboard_stats?.filterable_dashboard_data} | ||
| <Dashboard | ||
| data={dashboard_stats.filterable_dashboard_data} | ||
| programmingGoalsProgress={dashboard_stats | ||
| .programming_goals_progress || []} | ||
| onFiltersChange={refreshDashboardData} | ||
| /> | ||
| {/if} | ||
|
|
||
| {#if dashboard_stats?.activity_graph} | ||
| <ActivityGraph data={dashboard_stats.activity_graph} /> | ||
| {/if} | ||
| </div> |
There was a problem hiding this comment.
The dashboard markup is now duplicated between the dashboard_stats and <Deferred> branches (TodaySentence/Dashboard/ActivityGraph). To reduce drift and make future changes safer, consider extracting a shared snippet/component for the common render and reuse it in both branches.
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
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.
No description provided.