fix(googleicons): sort icons for glyph subsetting

[florian-lefebvre]

Closes #336

Summary by CodeRabbit

• Chores
  • Improved icon identifier formatting to produce stable, predictable, comma-separated, and consistently ordered lists for icons.
• Tests
  • Updated test snapshots and validation data to match the current provider behavior and identifiers.
• Chores
  • Maintains backward compatibility with the existing icon provider interface.

[coderabbitai]

📝 Walkthrough

Walkthrough

getFontDetails in the googleicons provider now sorts glyph names (via toSorted()) and joins them with commas when building the icon_names query parameter. Corresponding tests and optimizer identity snapshots were updated to match the new behavior.

Changes

Cohort / File(s)	Summary
Provider Logic src/providers/googleicons.ts	Constructs icon_names by sorting glyphs (toSorted()) and joining with , instead of '', producing a stable, comma-separated parameter.
Tests & Snapshots test/providers/googleicons.test.ts	Replaced experimental glyphs (['arrow_right','favorite','arrow_drop_down'] → ['bar_chart','bolt']) and updated optimizer identity inline snapshots to assert non-empty kit/skey values (new snapshot strings reflected).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I sorted my carrots, then joined them with care,
Commas between, tidy rows in the air.
Icons now sing in a stable parade,
No more mashed-together mishaps made. 🥕✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	The PR directly addresses issue #336 by changing join('') to join(',') in getFontDetails and updating test snapshots to reflect the corrected kit/skey values.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the googleicons subsetting issue: the join parameter fix and corresponding test updates.
Title check	✅ Passed	The title accurately describes the main change: sorting icons for glyph subsetting via the toSorted() addition before joining with commas to fix the subsetting issue.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/googleicons-glyphs

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.33%. Comparing base (8f7c1ee) to head (aa1bcbd).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #343   +/-   ##
=======================================
  Coverage   98.33%   98.33%           
=======================================
  Files          12       12           
  Lines         660      660           
  Branches      172      172           
=======================================
  Hits          649      649           
  Misses         11       11           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

test/providers/bunny.test.ts (1)

56-62: Avoid pinning Bunny’s current Alef catalog size.

Line 61 hard-codes the number of weights currently advertised upstream, but this test is really about the fallback behavior in prepareWeights(). If Bunny adds or removes Alef weights again, this will fail without any regression in src/providers/bunny.ts:18-35. Prefer asserting that the resolved weights are non-empty and all fall within the requested 400..1100 range, or stub the metadata for a deterministic count.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/providers/bunny.test.ts` around lines 56 - 62, The test currently pins
Bunny’s Alef catalog size (expect(fonts.length).toBe(2)) which makes it fragile;
update the test in test/providers/bunny.test.ts to assert that the resolved
fonts array is non-empty and that every returned font weight falls within the
requested 400..1100 range (or alternatively stub providers.bunny() metadata to a
deterministic set), i.e. keep using createUnifont and unifont.resolveFont but
replace the fixed-length assertion with checks that fonts.length > 0 and that
each font's weight parsed from the resolved result is >= 400 and <= 1100 (or
stub the provider metadata to assert an exact count).

test/providers/googleicons.test.ts (1)

75-87: Prefer asserting a stable subsetting signal over exact optimizer hashes.

Lines 81-82 and 121-122 snapshot opaque kit/skey values, so these tests will churn on unrelated optimizer/hash changes while telling us very little about the actual regression. Since the bug here is icon_names serialization, it would be more robust to assert a stable observable instead—e.g. that both code paths produce the same non-empty identity, or that the generated request encodes icon_names=bar_chart,bolt.

Also applies to: 115-127

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/providers/googleicons.test.ts` around lines 75 - 87, The snapshot
currently asserts exact opaque "kit"/"skey" values (in identifiersByFormat)
which will break on optimizer/hash churn; instead update the test that builds
identifiersByFormat to assert a stable property: verify that both code paths
produce the same non-empty identity (e.g.
identifiersByFormat.woff2[0].identifier is defined and equal across runs) and
add an assertion that the generated request payload or URL contains the expected
encoded icon_names string ("icon_names=bar_chart,bolt"); apply the same change
to the other similar block referenced around lines 115-127 so tests no longer
rely on exact kit/skey hashes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/providers/googleicons.ts`:
- Around line 47-48: The iconNames assembly uses caller-provided glyph arrays
without sorting, causing Google to return 400 for unsorted lists; update the
logic that builds iconNames (the expression using
options.options?.experimental?.glyphs and
providerOptions.experimental?.glyphs?.[family]) to first copy the glyph array
(if present), sort it deterministically (e.g., localeCompare or default string
sort), then join with ','—preserve the existing null/undefined behavior so
iconNames stays undefined when no glyphs are provided.

---

Nitpick comments:
In `@test/providers/bunny.test.ts`:
- Around line 56-62: The test currently pins Bunny’s Alef catalog size
(expect(fonts.length).toBe(2)) which makes it fragile; update the test in
test/providers/bunny.test.ts to assert that the resolved fonts array is
non-empty and that every returned font weight falls within the requested
400..1100 range (or alternatively stub providers.bunny() metadata to a
deterministic set), i.e. keep using createUnifont and unifont.resolveFont but
replace the fixed-length assertion with checks that fonts.length > 0 and that
each font's weight parsed from the resolved result is >= 400 and <= 1100 (or
stub the provider metadata to assert an exact count).

In `@test/providers/googleicons.test.ts`:
- Around line 75-87: The snapshot currently asserts exact opaque "kit"/"skey"
values (in identifiersByFormat) which will break on optimizer/hash churn;
instead update the test that builds identifiersByFormat to assert a stable
property: verify that both code paths produce the same non-empty identity (e.g.
identifiersByFormat.woff2[0].identifier is defined and equal across runs) and
add an assertion that the generated request payload or URL contains the expected
encoded icon_names string ("icon_names=bar_chart,bolt"); apply the same change
to the other similar block referenced around lines 115-127 so tests no longer
rely on exact kit/skey hashes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c49c775d-8cec-456a-a481-1bfa91e12814

📥 Commits

Reviewing files that changed from the base of the PR and between d1a58d7 and 3236b07.

📒 Files selected for processing (3)

• src/providers/googleicons.ts
• test/providers/bunny.test.ts
• test/providers/googleicons.test.ts

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/providers/googleicons.ts`:
- Around line 47-48: Replace the non-mutating sort pattern in the Google Icons
code by using Array.prototype.toSorted: update the expression that computes
iconNames (currently using (options.options?.experimental?.glyphs ??
providerOptions.experimental?.glyphs?.[family])?.slice().sort().join(',')) to
call .toSorted() on the source array before .join(',') so it avoids the
intermediate slice and satisfies the e18e/prefer-array-to-sorted rule (i.e., use
...?.toSorted().join(',') on the same glyphs access).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d29162b3-7c6a-4273-a607-7e0ccc6f1d9a

📥 Commits

Reviewing files that changed from the base of the PR and between 3236b07 and e045ded.

📒 Files selected for processing (2)

• src/providers/googleicons.ts
• test/providers/googleicons.test.ts