Skip to content

Conversation

@ainsleyclark
Copy link
Contributor

Summary

Upgrades svelte-hamburgers from v4.1.0 to v5.0.0 to fix Svelte 5 compatibility issues. The v4 package uses legacy Svelte 4 syntax that's incompatible with Svelte 5 runes mode.

Changes:

  • Upgraded dependency to svelte-hamburgers@^5.0.0
  • Updated event binding from on:change to onclick (Svelte 5 native events)
  • Enabled runes mode explicitly in svelte.config.js
  • Verified package builds successfully

Test plan

  • Build succeeds without errors
  • No new TypeScript errors introduced
  • Package compiles with Svelte 5 runes enabled

🤖 Generated with Claude Code

- Upgrade svelte-hamburgers from ^4.1.0 to ^5.0.0
- Update event binding syntax from on:change to onclick in Hamburger.svelte
- Enable runes mode in svelte.config.js for Svelte 5 runes compilation
- Verify build succeeds with new configuration

The svelte-hamburgers v5.0.0 is built specifically for Svelte 5 and uses native
event handlers instead of the legacy event binding syntax. Runes mode is now
explicitly enabled to ensure all components compile with Svelte 5 runes.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Jan 20, 2026

Review summary

  • Overall score: 7/10 (use scoring rubric below)
  • Critical issues: 0
  • Warnings: 1
  • Suggestions: 2
  • Recommendation: ⚠️ Approve with comments

This PR successfully upgrades svelte-hamburgers from v4 to v5 for Svelte 5 compatibility. The dependency upgrade and event handler migration are correct, but there's a timing issue with the onclick callback that could affect the consuming Sidebar component.

Critical issues 🔴

None

Warnings 🟡

Timing issue with onChange callback

Location: packages/sveltekit-helper/src/components/Hamburger.svelte:48-50

The onclick handler calls onChange?.(isOpen) with the current value of isOpen before the click is processed. However, in Svelte 5, the bind:open={isOpen} binding on the SvelteHamburger component will update isOpen after the click event fires.

This means onChange receives the old value instead of the new value:

  • If the menu is closed, clicking triggers onChange(false) then isOpen becomes true
  • If the menu is open, clicking triggers onChange(true) then isOpen becomes false

Impact: The Sidebar component at line 154 uses <Hamburger bind:isOpen />, so the state will eventually sync, but any callbacks relying on the correct state in onChange will receive incorrect values.

Suggested fix

Change the onclick handler to read the value after the state updates:

onclick={() => {
	// Use tick() or access the new value after the binding updates
	setTimeout(() => onChange?.(isOpen), 0);
}}

Or better yet, remove the onclick entirely and use a `` to watch isOpen:

(() => {
	onChange?.(isOpen);
});

This ensures the callback always receives the current state value.

Suggestions 🟢

Add tests for the Hamburger component

No tests were found for the Hamburger component. Given this is a breaking dependency upgrade with API changes, adding basic tests would help prevent regressions.

Suggested test cases:

  • Component renders with default props
  • isOpen binding works bidirectionally
  • onChange callback is invoked with correct state
  • CSS custom properties are applied correctly

Enable runes mode globally may have unintended consequences

Location: packages/sveltekit-helper/svelte.config.js:8-10

Enabling runes: true in compilerOptions affects all Svelte components in this package, not just the Hamburger component. While the package already uses runes mode ((), (), (), ()), explicitly enabling it is good for clarity.

However, consider documenting this decision in the PR description or adding a comment in the config file explaining why runes mode is required (e.g., "Required for svelte-hamburgers v5 compatibility").


Scoring guide (1-10):

  • 9-10: Production ready, excellent quality
  • 7-8: Minor improvements needed, safe to merge
  • 5-6: Notable issues but functional
  • 3-4: Significant problems requiring fixes
  • 1-2: Critical issues, do not merge

Score rationale: The upgrade is technically sound and the build passes, but the timing issue with the onChange callback is a functional bug that could cause unexpected behaviour in consumer code. The absence of tests is also a concern for such a breaking change. These issues should be addressed, though the PR is functional enough to merge with caution.

@ainsleyclark ainsleyclark merged commit 45fc51f into main Jan 20, 2026
4 checks passed
@ainsleyclark ainsleyclark deleted the fix/svelte-hamburgers-v5-compatibility branch January 20, 2026 07:59
@codecov
Copy link

codecov bot commented Jan 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.80%. Comparing base (7f6b060) to head (fe8c8e2).
⚠️ Report is 460 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #378      +/-   ##
==========================================
+ Coverage   64.59%   69.80%   +5.21%     
==========================================
  Files         154      185      +31     
  Lines        6064     7359    +1295     
==========================================
+ Hits         3917     5137    +1220     
+ Misses       2064     2025      -39     
- Partials       83      197     +114     

☔ 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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants