[#73737] Improve Backlogs and Sprints page performance by deferring menu load#22693
Conversation
f2493af to
f3f05da
Compare
f3f05da to
11323c8
Compare
11323c8 to
5a658ba
Compare
EinLama
left a comment
There was a problem hiding this comment.
This works great and improves performance nicely 👏 the page feels much snappier now.
I have two small points, please have a look and let me know what you think.
|
@dombesz it definitely makes sense to introduce deferred rendering of Action Menus at some point - it's a good, idiomatic solution, and the performance stats speak for themselves. However, as mentioned elsewhere, I would like us to think twice about where this lands -i.e. whether this really needs to go in
|
There was a problem hiding this comment.
Pull request overview
This PR improves the performance of the Backlogs/Sprints planning page by deferring rendering/loading of per-item action menus using Primer’s ActionMenu with src (lazy-loaded menu items), and adds a Turbo morph workaround to keep deferred menus functional after morph-based updates.
Changes:
- Convert inbox/story row menus to deferred
Primer::Alpha::ActionMenuinstances that load their menu list via newGET .../menuendpoints. - Add controller actions, routes, and permissions for the deferred menu fragments (
rb_stories#menu,inbox#menu) plus related routing/controller/component specs. - Add a frontend Turbo hook to remount deferred action menus after
turbo-streammorph replaces.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/backlogs/spec/routing/rb_stories_routing_spec.rb | Adds routing coverage for deferred story menu endpoint. |
| modules/backlogs/spec/routing/inbox_routing_spec.rb | Adds routing coverage for deferred inbox menu endpoint. |
| modules/backlogs/spec/lib/open_project/backlogs/permissions_spec.rb | Ensures view permission includes new menu fragment actions. |
| modules/backlogs/spec/controllers/rb_stories_controller_spec.rb | Adds controller specs for GET #menu authorization/content. |
| modules/backlogs/spec/controllers/inbox_controller_spec.rb | Adds controller specs for GET #menu authorization/content. |
| modules/backlogs/spec/components/backlogs/story_menu_component_spec.rb | Updates expectations to reflect deferred-list rendering (no show button in fragment). |
| modules/backlogs/spec/components/backlogs/story_component_spec.rb | Verifies deferred action menu host includes include-fragment and stable ids. |
| modules/backlogs/spec/components/backlogs/inbox_menu_component_spec.rb | Updates expectations to reflect deferred-list rendering (no show button in fragment). |
| modules/backlogs/spec/components/backlogs/inbox_item_component_spec.rb | Verifies deferred action menu host includes include-fragment and stable ids. |
| modules/backlogs/spec/components/backlogs/inbox_component_spec.rb | Updates component construction due to removed args. |
| modules/backlogs/lib/open_project/backlogs/engine.rb | Registers new controller actions under permissions. |
| modules/backlogs/config/routes.rb | Adds menu member routes for inbox and stories. |
| modules/backlogs/config/locales/en.yml | Moves label_actions translation to the component that renders the show button. |
| modules/backlogs/app/views/rb_master_backlogs/_backlog_list.html.erb | Removes now-unneeded open_sprints_exist passing. |
| modules/backlogs/app/controllers/rb_stories_controller.rb | Adds menu action rendering deferred story menu fragment. |
| modules/backlogs/app/controllers/inbox_controller.rb | Adds menu action rendering deferred inbox menu fragment. |
| modules/backlogs/app/components/backlogs/story_menu_component.rb | Refactors to render only the deferred menu list and expose menu_id. |
| modules/backlogs/app/components/backlogs/story_menu_component.html.erb | Switches from full ActionMenu to ActionMenu::List fragment. |
| modules/backlogs/app/components/backlogs/story_component.rb | Drops max_position dependency (now computed in menu endpoint). |
| modules/backlogs/app/components/backlogs/story_component.html.erb | Renders deferred ActionMenu host with src and show button. |
| modules/backlogs/app/components/backlogs/sprint_component.rb | Removes max_position helper previously used by menus. |
| modules/backlogs/app/components/backlogs/sprint_component.html.erb | Stops passing max_position into story rows. |
| modules/backlogs/app/components/backlogs/inbox_menu_component.rb | Refactors to render only the deferred menu list and expose menu_id. |
| modules/backlogs/app/components/backlogs/inbox_menu_component.html.erb | Switches from full ActionMenu to ActionMenu::List fragment. |
| modules/backlogs/app/components/backlogs/inbox_item_component.rb | Drops max_position/open_sprints_exist dependency (now computed in menu endpoint). |
| modules/backlogs/app/components/backlogs/inbox_item_component.html.erb | Renders deferred ActionMenu host with src and show button. |
| modules/backlogs/app/components/backlogs/inbox_component.rb | Removes open_sprints_exist and max_position helper. |
| modules/backlogs/app/components/backlogs/inbox_component.html.erb | Stops passing removed args into inbox row collection. |
| modules/backlogs/app/components/backlogs/backlog_component.rb | Removes max_position helper previously used by menus. |
| modules/backlogs/app/components/backlogs/backlog_component.html.erb | Stops passing max_position into story rows. |
| frontend/src/turbo/setup.ts | Registers the new action-menu Turbo morph workaround. |
| frontend/src/turbo/action-menu-stream-actions.ts | Adds Turbo morph hook to remount deferred Primer action menus. |
myabc
left a comment
There was a problem hiding this comment.
In additional to my first comment on where this PR lands, here's a more in-depth code review:
The good
Big kudos for trying to fix this globally - and in a clear, reusable manner. We will need a fix in many other places.
The not so good
- Hooking into wrong action:
turbo:morph-elementis a better fit, because you can target only morphedaction-menuelements and remount them after morph, regardless of whether the morph came from a stream, frame, or page refresh. - Test coverage: The added specs prove the deferred HTML/routes, but not the failure mode: "menu still works after a morph update". Ideally, there would be a Capybara spec that triggers a Backlogs morph and then opens a deferred menu, otherwise this could regress silently.
Alternatives
You could potentially just use data-turbo-permanent to keep the whole action-menu around. The danger here is that would preserve already-loaded - and potentially stale - menu state (this could be mitigated though by re-implementing the move item actions on the client-side).
0aaf2ba to
6580f26
Compare
|
Thanks Tobi and Alex for the review, You brought up some very good points!
Changed to
There are already specs that test the menu interaction here and here that will fail if the menu is not correctly loaded. This spec run has failed before the fix.
I have considered this option, but discarded for the same reason you just mentioned. Edit: The commits will be squashed right before merging. I believe I addressed all your comments, please have a re-review, thanks! |
…cessed if it's not part of the requested story or project. Add menu route to the new routes.
6580f26 to
25a7380
Compare
Ticket
https://community.openproject.org/wp/73737
What are you trying to accomplish?
What approach did you choose and why?
Primer::Alpha::ActionMenusfor the Inbox and the Sprint work packages.Performance results
Loading time is reduced from average 8.41s to 4.154s, which is a 50.61 % speedup🔺. (Combined with the #22659, is a ~61% improvement.)
Payload reduced from 4.34MB to 529kB, a ~88% 🔻 reduction in payload size, which also leads to a reduction of the frontend processing time.
The test was done having 1000 work packages in the inbox in dev environment.
Merge checklist