[OP-19772] Load projects asynchronously in the project selector by HDinger · Pull Request #25387 · opf/openproject

HDinger · 2026-09-16T13:20:02Z

Ticket

https://community.openproject.org/wp/OP-19772

What are you trying to accomplish?

Use async TreeView within the initial loading state of the project selector. The initial state should only show the root level projects. Children are only loaded when the parent is expanded.

AI involvement

Collaborative – AI generated a substantial part of the code; I reviewed and understand every line

github-actions · 2026-09-16T13:48:56Z

Warning

Flaky specs

rspec ./spec/features/notifications/navigation_spec.rb[1:1:1]
rspec ./spec/features/projects/creation_wizard/wizard_from_template_flow_spec.rb[1:1]
rspec ./spec/features/roles/report_spec.rb[1:1]
rspec ./spec/features/roles/report_spec.rb[1:2]
rspec ./spec/features/roles/report_spec.rb[1:3]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #25387, linked for reference only):

- `rspec ./spec/features/notifications/navigation_spec.rb[1:1:1]`
- `rspec ./spec/features/projects/creation_wizard/wizard_from_template_flow_spec.rb[1:1]`
- `rspec ./spec/features/roles/report_spec.rb[1:1]`
- `rspec ./spec/features/roles/report_spec.rb[1:2]`
- `rspec ./spec/features/roles/report_spec.rb[1:3]`

Treat this as a standalone task, unrelated to PR #25387. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #25387 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @HDinger to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @HDinger, and request a review from @HDinger.
On every commit, set @HDinger as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

github-actions · 2026-09-17T15:22:28Z

Warning

Flaky specs

rspec ./spec/features/notifications/navigation_spec.rb[1:1:1]
rspec ./spec/features/projects/creation_wizard/wizard_from_template_flow_spec.rb[1:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #25387, linked for reference only):

- `rspec ./spec/features/notifications/navigation_spec.rb[1:1:1]`
- `rspec ./spec/features/projects/creation_wizard/wizard_from_template_flow_spec.rb[1:1]`

Treat this as a standalone task, unrelated to PR #25387. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #25387 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @HDinger to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @HDinger, and request a review from @HDinger.
On every commit, set @HDinger as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

HDinger · 2026-09-18T08:37:42Z

Hi @NobodysNightmare may I ask you for a review on this, given that you worked on the previous implementation and understand the problem scope already very well?

NobodysNightmare

I believe to have spotted errors. Maybe they turn out to be errors in my understanding, let's see :D

Otherwise mostly questions and small improvement ideas from my side. I didn't click-test it yet.

NobodysNightmare · 2026-09-28T12:29:40Z

-    (projects + current.self_and_ancestors.visible.active.to_a).uniq(&:id).sort_by(&:lft)
+    ancestors = current.self_and_ancestors.visible.active.to_a
+    ancestor_children = (ancestors - [current]).flat_map do |ancestor|
+      Project.nearest_visible_descendants(ancestor, limit: MAX_NUMBER_OF_PROJECTS).to_a


Quite unfortunate that we have to loop over the ancestors here. This means one SQL query per depth of the current project.

I think we can't do anything here, but a question I have (and maybe can answer before submitting the review): Would it be possible for us to pass all the ancestors into nearest_visible_descendants to get the desired result?

Mh, so if I understand nearest_visible_descendants correctly, it should be able to deal with multiple inputs for the "nearest visible" part. If we have an efficient equivalent to Array(boundary).flat_map(&:descendants), we'd be able to avoid the Ruby-side loop entirely.

NobodysNightmare · 2026-09-28T13:20:25Z

+    # Used to decide whether a node gets an expand arrow or renders as a leaf
+    # has_subprojects?/leaf? is not enough, as it only looks at lft/rgt and would count an archived or invisible
+    # descendant as "has children" too.
+    def with_visible_descendants(candidates)


🟢 I know naming is hard, but the first time I read the call to this method I thought it would return the projects together with their visible descendants until I realized that this would be the method called nearest_visible_descendants and then I was confused.

My only alternative idea would be Project.having_visible_descendants(projects). I tried to come up with more than one proposal, but I didn't like the others at all.

NobodysNightmare · 2026-09-28T14:13:33Z

+    def visible_active_ids_within(candidates)
+      min_lft = candidates.map(&:lft).min
+      max_rgt = candidates.map(&:rgt).max
+      Project.visible.active.where("lft > ? AND rgt < ?", min_lft, max_rgt).pluck(:id)


Can't this list of projects become uncomfortably large? From what I can see we resolve it directly into a Ruby array (pluck), so it's not going to stay a subquery in the database.

I guess there's a reason behind it, but can't we make sure to keep it as a subquery?

…lector. The initial state should only show the root level projects. Children are only loaded when the parent is expanded.

… elements within the lft/rgt ranges

Initially, the set of visible projects was in a CTE referenced twice in the query, which led to the CTE being manifested. In this case, this is iefficient as the index on lft/rgt cannot be used. But even when preventing the manifestation, the comparison within an EXIST led to the two instances of the set needing to be looped through (nested loop anti join) effectively resulting in the cross product being considered. The solution here is to rely on the awesome_nested_set's algorithm to effectively calculate whether a project has visible ancestors which can only be those preceeding in a list ordered by lft.

github-actions · 2026-09-29T13:40:04Z

Warning

Flaky specs

rspec ./modules/gantt/spec/features/timeline/timeline_dates_spec.rb[1:2:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #25387, linked for reference only):

- `rspec ./modules/gantt/spec/features/timeline/timeline_dates_spec.rb[1:2:1]`

Treat this as a standalone task, unrelated to PR #25387. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #25387 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @HDinger to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @HDinger, and request a review from @HDinger.
On every commit, set @HDinger as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

NobodysNightmare · 2026-09-29T14:06:04Z

+        end
+      end
+
+      it "respects the limit and orders by lft" do


Surprised that there's no rubocop-rspec rule for that: I find it surprising to have an it after a context declaration. We now have 3 its followed by a context then followed by an it.

Can we group the its together (usually at the top)?

NobodysNightmare · 2026-09-29T14:08:43Z

+    end
+
+    context "with a boundary" do
+      shared_let(:parent) { create(:project, name: "Parent") }


🟡 Considering the possible classes of errors, I'd probably also include an other_parent, just to make sure that this one doesn't pop up in the results.

On a higher level: Theoretically we should be able to share the same hierarchy setup between the "with boundary" and "without boundary" test cases. This would essentially solve the problem as well.

NobodysNightmare · 2026-09-29T14:12:35Z

+
+  describe ".with_visible_descendants" do
+    shared_let(:parent) { create(:project, name: "Parent") }
+    shared_let(:child) { create(:project, name: "Child", parent:) }


I am surprised that we care to create child in the setup, but never check whether it could become part of the result. I think we should check that.

NobodysNightmare · 2026-09-29T14:13:40Z

+    end
+
+    context "when the only descendant is archived" do
+      shared_let(:archived_parent) { create(:project, name: "Archived Parent") }


🟡 Confusing name: The parent is not archived, its child is.

Suggested change

shared_let(:archived_parent) { create(:project, name: "Archived Parent") }

shared_let(:parent_of_archived) { create(:project, name: "Archived Parent") }

HDinger added this to the 18.0.x milestone Sep 16, 2026

HDinger added the feature label Sep 16, 2026

HDinger marked this pull request as ready for review September 18, 2026 08:36

HDinger requested a review from NobodysNightmare September 18, 2026 08:36

HDinger added the needs review label Sep 18, 2026

HDinger force-pushed the feature/op-19772-load-projects-asynchronously-in-the-project-selector branch from 658047f to 690e61e Compare September 22, 2026 11:11

github-actions Bot added the ai: Collaborative 💻 AI generated a substantial part of the code; A human reviewed and understands every line. label Sep 22, 2026

NobodysNightmare requested changes Sep 28, 2026

View reviewed changes

HDinger and others added 4 commits September 29, 2026 15:00

Use async TreeView within the initial loading state of the project se…

2b66399

…lector. The initial state should only show the root level projects. Children are only loaded when the parent is expanded.

Replace nearest_visible_descendants with a single query to search for…

74b9700

… elements within the lft/rgt ranges

Load the complete upwards tree not only direct ancestors

4c2c7d6

HDinger force-pushed the feature/op-19772-load-projects-asynchronously-in-the-project-selector branch from 690e61e to 7494987 Compare September 29, 2026 13:08

Add tests for the hierarchy helper

07c37ca

HDinger force-pushed the feature/op-19772-load-projects-asynchronously-in-the-project-selector branch from 7494987 to 07c37ca Compare September 29, 2026 13:13

NobodysNightmare reviewed Sep 29, 2026

View reviewed changes

	shared_let(:archived_parent) { create(:project, name: "Archived Parent") }
	shared_let(:parent_of_archived) { create(:project, name: "Archived Parent") }

Conversation

HDinger commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

AI involvement

Uh oh!

github-actions Bot commented Sep 16, 2026

Uh oh!

github-actions Bot commented Sep 17, 2026

Uh oh!

HDinger commented Sep 18, 2026

Uh oh!

NobodysNightmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

HDinger commented Sep 16, 2026 •

edited

Loading