fix: use varying widths for compare bar chart skeleton loaders#2209
fix: use varying widths for compare bar chart skeleton loaders#2209thealxlabs wants to merge 3 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughThe PR updates the Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
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 |
![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)
app/components/Compare/FacetBarChart.vue (1)
336-336: Extract the width formula to a shared helper to avoid drift.The same inline formula appears in two places (Line 336 and Line 352). Keeping it in one helper will prevent accidental divergence between fallback branches.
♻️ Proposed refactor
<script setup lang="ts"> +const skeletonBarBaseWidth = 40 +const skeletonBarStep = 17 +const skeletonBarRange = 40 + +function getSkeletonBarWidth(index: number): string { + return `${skeletonBarBaseWidth + ((index * skeletonBarStep) % skeletonBarRange)}%` +} </script>-<SkeletonInline class="h-3 shrink-0" :style="{ width: `${40 + ((i * 17) % 40)}%` }" /> +<SkeletonInline class="h-3 shrink-0" :style="{ width: getSkeletonBarWidth(i) }" />Also applies to: 352-352
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7610e178-e7ce-436b-846a-30a42384f02a
📒 Files selected for processing (2)
app/components/Compare/FacetBarChart.vuetest/nuxt/components/compare/FacetBarChart.spec.ts
| it('skeleton bar label widths vary across packages (not all the same)', () => { | ||
| // The formula used is: width = `${40 + (i * 17) % 40}%` | ||
| // For 3 packages: i=0 -> 40%, i=1 -> 57%, i=2 -> 74% | ||
| const packages = ['react', 'vue', 'svelte'] | ||
| const widths = packages.map((_, i) => `${40 + ((i * 17) % 40)}%`) | ||
|
|
||
| // Verify they are not all the same (the bug was all bars had the same width) | ||
| const uniqueWidths = new Set(widths) | ||
| expect(uniqueWidths.size).toBeGreaterThan(1) | ||
| }) |
There was a problem hiding this comment.
Regression test is detached from the component output.
Line 30 to Line 48 currently re-tests arithmetic in the spec itself, so it can still pass if the template no longer applies those widths. Please assert widths from rendered skeleton DOM/styles instead.
✅ Proposed test rewrite (DOM-coupled)
- it('skeleton bar label widths vary across packages (not all the same)', () => {
- // The formula used is: width = `${40 + (i * 17) % 40}%`
- // For 3 packages: i=0 -> 40%, i=1 -> 57%, i=2 -> 74%
- const packages = ['react', 'vue', 'svelte']
- const widths = packages.map((_, i) => `${40 + ((i * 17) % 40)}%`)
-
- // Verify they are not all the same (the bug was all bars had the same width)
- const uniqueWidths = new Set(widths)
- expect(uniqueWidths.size).toBeGreaterThan(1)
- })
-
- it('skeleton width formula produces values within 40-80% range', () => {
- const indices = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
- for (const i of indices) {
- const width = 40 + ((i * 17) % 40)
- expect(width).toBeGreaterThanOrEqual(40)
- expect(width).toBeLessThan(80)
- }
- })
+ it('renders varying skeleton label widths within 40–79% for package rows', async () => {
+ const wrapper = await mountSuspended(FacetBarChart, { props: baseProps })
+ const rows = wrapper.findAll('div.flex.items-center.gap-3')
+ const widths = rows.map((row) => {
+ const style = row.find('span.h-3.shrink-0').attributes('style') || ''
+ const match = style.match(/width:\s*(\d+(?:\.\d+)?)%/)
+ return Number(match?.[1])
+ })
+
+ expect(new Set(widths).size).toBeGreaterThan(1)
+ for (const width of widths) {
+ expect(Number.isFinite(width)).toBe(true)
+ expect(width).toBeGreaterThanOrEqual(40)
+ expect(width).toBeLessThan(80)
+ }
+ })As per coding guidelines, "Write unit tests for core functionality using vitest."
Also applies to: 41-48
| const rowMatches = html.match(/flex items-center gap-3/g) | ||
| expect(rowMatches).not.toBeNull() | ||
| }) |
There was a problem hiding this comment.
Assert exact row count, not only presence.
Line 63 only checks that at least one match exists. To verify “one row per package”, assert the exact number of rendered rows.
🎯 Tightened assertion
- const rowMatches = html.match(/flex items-center gap-3/g)
- expect(rowMatches).not.toBeNull()
+ const rowMatches = html.match(/flex items-center gap-3/g) ?? []
+ expect(rowMatches).toHaveLength(3)📝 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.
| const rowMatches = html.match(/flex items-center gap-3/g) | |
| expect(rowMatches).not.toBeNull() | |
| }) | |
| const rowMatches = html.match(/flex items-center gap-3/g) ?? [] | |
| expect(rowMatches).toHaveLength(3) | |
| }) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ghostdevv
left a comment
ghostdevv
left a comment
There was a problem hiding this comment.
Could you take a look at the coderabbit suggestions, and also show maybe a screenshot of the before/after?
2 participants
🔗 Linked issue
resolves #2106
🧭 Context
The compare page FacetBarChart skeleton loaders showed all bars at the same fixed width, giving an unrealistic loading placeholder. Real bar charts show bars of varying widths corresponding to different values.
📚 Description
Updated the skeleton loader in
Compare/FacetBarChart.vueto compute per-bar widths usingwidth: ${40 + (i * 17) % 40}%so adjacent bars have different lengths, producing a more realistic loading state. Also addedaria-hidden="true"to skeleton containers and updated the heading/subtitle skeletons to more realistic widths.A regression test (
test/nuxt/components/compare/FacetBarChart.spec.ts) verifies that the width formula produces different values across packages and that all widths stay within the expected 40–79% range.