feat(frontend): major bundle optimization and performance improvements - #19
feat(frontend): major bundle optimization and performance improvements#19ryanbas21 wants to merge 10 commits into
Conversation
|
Thanks for this! A lot of good stuff in here, but I'm not comfy merging it all as one PR. Would you be willing to split it into smaller PRs so I can test each change? Thanks! |
|
I'm really intrigued by this PR, but it's just sooooooooo huge. Even if I was to just farm it out to Claude or something, it would burn my entire usage window. |
I will say most of the changes are the lodash file renames. but I can look to change it up a bit |
15aaa97 to
750c20d
Compare
|
@robertlordhood @nickwolf I broke them out into individual commits (agent did yay). Let me know if this makes it more reviewable. I understand they build on each other, and it's still a lot of files. If you get past the lodash renaming then you basically cut this in half and hopefully individual commits helps. If not, I can break these into separate PR's. |
750c20d to
a756dd6
Compare
|
I went ahead and burned the usage window on it. The individual commits did help, so thanks for that. Two things came out of it: the lodash framing holds up by file count but not by lines, and there are four bugs worth fixing before this merges. On the file count you're right. 120 of the 245 files are touched only by the lodash commit, so "cut this in half" is fair as far as that goes. By lines it's much smaller. That commit is 965 lines across frontend/src, plus 15 in babel.config.js and webpack.config.js, out of 8,080 non-lockfile lines in the PR. So the renames are about 12% of the reviewable diff, not half. The 10,500 line total is also inflated by 2,420 lines of yarn.lock, and 2,381 of those sit inside the lodash commit, which is most of why it looks so big. The review time is really in the jQuery to fetch commit (2,765 lines in frontend/src) and react-virtualized to react-window (2,411). Those two are 65% of the source churn and neither is renaming. The commit boundaries also don't match the commit messages, which is what made this hard to follow: QueueRow.js (873 lines), HistoryRow.js (410), BlocklistRow.js (315), AuthorEditorRow.js (225) and IgnoredDownloadRow.js (178) are class to functional component conversions sitting inside the commit titled "replace jQuery AJAX with native fetch", and the new 123-line sw.js is inside the one titled "add React Query for server state management". Four bugs, all in the parts I actually read:
Two smaller ones: jquery 3.7.1 is still in package.json line 58 with no imports left in frontend/src, and the sw.js comment at line 59 flags sub-path urlBase deployments as unhandled and needing live-server testing, so that's worth pinning down before it ships, though subdomain installs at the root are unaffected. What I did not look at: the react-window migration, the 37 lazy routes and the react-query commit, which is 3,791 lines between them. Treat it as unreviewed. It's also the part most likely to change behaviour in ways that clicking through wouldn't catch. The Build Chaptarr workflow has also been sitting at action_required on both pushes, so someone needs to approve the run before CI has said anything either way. Not my repo and not my call, but if jQuery to fetch and react-window went out as their own PRs with the row conversions kept separate, I'd happily review both properly. Is there a reason the two need to land together? |
I can drop the jquery and the react window commits and make a separate pr for them. will do later tonight. |
…ry Deferred compat React Query commit used .catch() on createAjaxRequest().request which is a jQuery Deferred on the develop base (no .catch method). Wrapping with Promise.resolve assimilates the Deferred into a native Promise so .catch works.
moment is replaced by dayjs; drop the now-unused moment dependency. react-virtualized and jquery are retained.
a756dd6 to
81f8bc2
Compare
|
Alrighty - sorry for the files i got it down dropped those commits and fixed the tsc issues and the dayjs issue :) Note: I'm happy to squash these commits whenever this is ready obviously leaving the commits as such now for reviews and to not ruin history. |
Bundle size: 3.17MB → 3.39MB raw (+6.9%), 884KB → 948KB gzipped (+7.1%)
Initial load: 530KB → 339KB gzipped (−36.1% reduction)
Dependency migrations:
Bug fixes:
isBetweenplugin (used byBookFileRowdate range rendering)booksImportedObservable invalues()to emit array contents instead of the array objectPromise.resolve()for React Query compatibilitywebworkertsconfig with ambientprocesstype declaration (typecheck green)Webpack optimizations:
React performance fixes:
Build config:
Service Worker:
Some future looking ideas:
How was this tested?
Ran the app via docker, nothing seemed broken clicking through worked.
Screenshots (UI changes only)
A note on AI: We know AI/agentic coding is everywhere and only getting
more popular. We won't insist that you disclose whether you used it or which
models you used, but in the same spirit, please don't take offense if your PR
is scrutinized and changes are requested.
Review time: The longer the PR and the more lines changed, the longer the
review will take. Small, focused PRs merge fastest. If yours is big, please be
patient.