Bundle size optimization and hash handling fixes#25
Conversation
- Remove debug console.log from scroll-restoration.ts - Remove dead code (if (save) wrapper) in scroll-restoration.ts - Fix href getter to include hash when constructing from path array - Forward hash when reconstructing NavOpts in redirects - Replace new Array<T>() with [] for smaller compiled output - Add sideEffects: false to package.json for better tree-shaking - Add tests for href with hash
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 24 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis pull request increments the Changes
Possibly related PRs
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/esroute/src/nav-opts.ts (1)
52-53:⚠️ Potential issue | 🟠 Major
hrefcache can ignore explicitopts.hashoverrides.When constructed from an
hrefplus{ hash },_hmay be cached before the override is applied, sohrefreturns the old fragment.🔧 Proposed fix
- if (!search) this._h = href; + // Cache only when no explicit hash override is provided. + if (!search && hash == null) this._h = href;Also applies to: 63-63, 73-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/esroute/src/nav-opts.ts` around lines 52 - 53, The cached href stored in this._h can be set from the original href before applying an explicit opts.hash override, causing href to return the old fragment; update the logic in the constructor/initializer that sets this._h so it incorporates opts.hash (or clears/overwrites the fragment) after parsing href (the code around the variables href, _h, search, pathString, searchString, hash) — specifically, when opts.hash is present compute or rebuild the final href using pathString and searchString plus opts.hash (or assign this._h = rebuiltHref) instead of keeping the original href; apply the same adjustment to the other similar spots indicated (the blocks around the lines referenced at 63 and 73–77) so caching always reflects any explicit opts.hash override.
🧹 Nitpick comments (1)
packages/esroute/src/nav-opts.spec.ts (1)
100-115: Add a regression case forhref+ explicithashoverride.Current additions are good, but they don’t cover
new NavOpts("/foo#old", { hash: "new" }), which is where stale href caching can regress.🧪 Suggested test
describe("href with hash", () => { @@ it("should include hash in href when constructed from StrictNavMeta", () => { const opts = new NavOpts({ path: ["foo"], hash: "bar" }); expect(opts.href).toBe("/foo#bar"); }); + + it("should prefer explicit opts.hash over hash parsed from href", () => { + const opts = new NavOpts("/foo#old", { hash: "new" }); + expect(opts.href).toBe("/foo#new"); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/esroute/src/nav-opts.spec.ts` around lines 100 - 115, Add a regression test to ensure NavOpts correctly applies an explicit hash override when constructed from a string that already contains a hash: create a test that constructs NavOpts with a base like "/foo#old" and options { hash: "new" } and assert opts.href === "/foo#new"; target the NavOpts constructor behavior and its href property to catch stale caching of the original hash (referencing NavOpts and href).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/esroute/src/scroll-restoration.ts`:
- Around line 35-36: Change the visibilitychange listener to be attached to
document instead of window: locate the lines that add the event listeners (the
save function is used in window.addEventListener("beforeunload", save) and
window.addEventListener("visibilitychange", save)) and replace the
visibilitychange registration so it calls
document.addEventListener("visibilitychange", save) while leaving beforeunload
on window unchanged; ensure you only update the listener attachment target and
keep the save handler unchanged.
---
Outside diff comments:
In `@packages/esroute/src/nav-opts.ts`:
- Around line 52-53: The cached href stored in this._h can be set from the
original href before applying an explicit opts.hash override, causing href to
return the old fragment; update the logic in the constructor/initializer that
sets this._h so it incorporates opts.hash (or clears/overwrites the fragment)
after parsing href (the code around the variables href, _h, search, pathString,
searchString, hash) — specifically, when opts.hash is present compute or rebuild
the final href using pathString and searchString plus opts.hash (or assign
this._h = rebuiltHref) instead of keeping the original href; apply the same
adjustment to the other similar spots indicated (the blocks around the lines
referenced at 63 and 73–77) so caching always reflects any explicit opts.hash
override.
---
Nitpick comments:
In `@packages/esroute/src/nav-opts.spec.ts`:
- Around line 100-115: Add a regression test to ensure NavOpts correctly applies
an explicit hash override when constructed from a string that already contains a
hash: create a test that constructs NavOpts with a base like "/foo#old" and
options { hash: "new" } and assert opts.href === "/foo#new"; target the NavOpts
constructor behavior and its href property to catch stale caching of the
original hash (referencing NavOpts and href).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61a81ce8-9d60-4beb-a01f-61db282ad358
📒 Files selected for processing (7)
packages/esroute-lit/package.jsonpackages/esroute/package.jsonpackages/esroute/src/nav-opts.spec.tspackages/esroute/src/nav-opts.tspackages/esroute/src/route-resolver.tspackages/esroute/src/router.tspackages/esroute/src/scroll-restoration.ts
Adds a Keep a Changelog formatted changelog documenting all releases from v0.1.0 through v0.12.1, including features, fixes, and breaking changes for each version.
1 participant
Summary
Changes
console.logdebug statement from scroll-restorationif (save)check) in scroll-restorationnew Array<T>()with[]for smaller compiled outputsideEffects: falseto both package.json files for better tree-shakingTest plan
npm run buildsucceedsSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores