feat(rollups): add rollup_contents mapping #519

Carbonhell · 2025-12-22T00:33:13Z

Closes #494.

I wasn't sure whether to use IDs from the pull_request table to represent PRs in the mapping, or just the GitHub PR number - I went with the latter as there would be no record for the rollup at the time of its creation, as that gets populated via GitHub events, if I understood correctly.

I haven't experienced rollups at all as I haven't (yet?) contributed to a project with bors, but I noticed there's no safety check for multiple rollups containing the same PR(s) being opened. I found a related comment here, but I kept it out of this PR to reduce its scope and simplify review. I think such a safety check could be added easily by checking for rollups associated to each candidate PR, and verifying the status of the rollup PR to ignore closed ones. That would still allow one to recreate the same scenario by reopening a previously closed rollup, though.

One thing I'm unsure about is how to handle the error when writing the rollup_contents records in the DB. I think that should only happen either in case of DB connection errors, or if the list of PRs contains duplicates. I assume the former can be propagated to the caller, whereas I'm not sure if the latter is realistic and should be accounted for. I'm thinking we can just ignore the conflicting records in such case - let me know if I should proceed with that.

Edit: sorry, I forgot the migration test files. Will push them tomorrow as soon as possible

Kobzol · 2025-12-22T10:24:45Z

Hi, thanks for the PR! I'll take a look, but I don't know when I'll be able to get to it, since it's Holiday season :)

I wasn't sure whether to use IDs from the pull_request table to represent PRs in the mapping, or just the GitHub PR number - I went with the latter as there would be no record for the rollup at the time of its creation, as that gets populated via GitHub events, if I understood correctly.

Yeah, that makes sense.

but I noticed there's no safety check for multiple rollups containing the same PR(s) being opened.

This is not a problem on its own, there are often multiple rollups that contain a single PR, because if a rollup fails, you create a new one, often before the old one is closed. I don't think that we need to think about that right now.

if the list of PRs contains duplicates. I assume the former can be propagated to the caller, whereas I'm not sure if the latter is realistic and should be accounted for.

Also a good point! I'll ignore duplicate PR numbers in rollups.

Kobzol

I think that the main query that we will want to execute on the queue page would look something like this:

SELECT
    rollup_pr_number, member_pr_number
FROM rollup_member
WHERE repository = $1 AND
      rollup_pr_number IN (SELECT number
                           FROM pull_request AS pr
                           WHERE pr.repository = $1
                             AND pr.status IN ('open', 'draft'))
ORDER BY rollup_pr_number;

And we get back something like HashMap<PrNum, HashSet<PrNum>>, which maps rollups to their member PRs. Because we assume that there will be only a couple of open rollups, so it seems easier and faster to do it this way, rather than try to find a corresponding rollup for each PR in the queue.

.gitattributes

src/database/client.rs

migrations/20251221160635_rollup_contents.up.sql

src/database/client.rs

migrations/20251221160635_rollup_contents.up.sql

Carbonhell · 2026-01-06T23:37:28Z

Sorry for the long delay (and the force-pushes 👀), the PR should be ready for another review now. Thank you in advance :)

Kobzol

Thank you, it looks good!

src/github/rollup.rs

Kobzol · 2026-01-07T10:08:52Z

I wouldn't actually mind having some basic usage of the new queries in the same PR - it often turns out that a new API will need to be changed once it's actually used 😆 Would you like to take a stab at that?

Some ideas:

If a rollup is currently being tested (it has state pending), it should not be possible to add its members to a new rollup (the rollup checkboxes should be disabled for the members)
Each rollup in the queue could have some color associated to it, and we could show some small rectangle or circle before/after the rollup PR number or title, in the queue table. And then we could also add this rectangle/circle at the same spot for the rollup members, to make it visually obvious which members belong to which rollups.
When you hover on a rollup, its members could be highlighted.

I think that 3. might be the simplest to implement, and it would be enough to test that the queries that we have here make sense. The rest could be done in follow-up PRs.

Carbonhell · 2026-01-07T21:31:05Z

I wouldn't actually mind having some basic usage of the new queries in the same PR - it often turns out that a new API will need to be changed once it's actually used 😆 Would you like to take a stab at that?

Some ideas:

If a rollup is currently being tested (it has state pending), it should not be possible to add its members to a new rollup (the rollup checkboxes should be disabled for the members)

Each rollup in the queue could have some color associated to it, and we could show some small rectangle or circle before/after the rollup PR number or title, in the queue table. And then we could also add this rectangle/circle at the same spot for the rollup members, to make it visually obvious which members belong to which rollups.

When you hover on a rollup, its members could be highlighted.

I think that 3. might be the simplest to implement, and it would be enough to test that the queries that we have here make sense. The rest could be done in follow-up PRs.

Honestly I agree on implementing the usage of the new queries in this PR. I've been sticking to small PRs mainly to facilitate the reviews and to avoid making it too time consuming for maintainers, as I'll probably do a few mistakes since I just started contributing :) I'd be more than happy to handle it! I'll look into it in this weekend

Kobzol · 2026-01-08T16:50:54Z

Yeah ofc small PRs are good in general. On the other hand, I tend to be a bit wary of PRs that just add new functionality without using it, because very often when we start using it we realize that we need to make changes to it anyway 😆

…nsert/get logic chore(git): add relevant gitattributes to ensure correct line endings for sqlx files

…ove unnecessary index tweak(rollups): replace access patterns to rollup_member with a single get_nonclosed_rollups operation, to be used within the queue page endpoint logic in the future fix(rollups): add missing test sql file chore(rollups): update migration timestamp

src/templates.rs

…en hovering the row representing a rollup PR

… is to verify the acceptable case of multiple rollups may point to the same member PR

Carbonhell · 2026-01-11T22:59:32Z

As agreed on Zulip, for now I've only implemented the hover functionality, as seen in the following screenshot (mouse hovering on a rollup not shown):

I've also fixed the issue with the multiple_rollups_same_pr test from the last review.
The PR should be ready for review. Thanks in advance :)

Kobzol

Left a couple of small nits, but otherwise this looks really awesome! The hover also looks and works great ❤️ Thank you!

Kobzol · 2026-01-12T10:17:38Z

migrations/20260106212918_rollup_member.up.sql

+    rollup_pr_number  BIGINT NOT NULL,
+    member_pr_number  BIGINT NOT NULL,
+    repository TEXT   NOT NULL,


Suggested change

rollup_pr_number BIGINT NOT NULL,

member_pr_number BIGINT NOT NULL,

repository TEXT NOT NULL,

repository TEXT NOT NULL,

rollup_pr_number BIGINT NOT NULL,

member_pr_number BIGINT NOT NULL,

Just a nit, it's easier to read like this when inspecting the DB state manually :) The migration data SQL file will also need to be updated after this change.

Kobzol · 2026-01-12T10:18:37Z

src/database/client.rs

+        Ok(())
+    }
+
+    /// Returns a map of rollup PR numbers to the set of member PR numbers that are part of that rollup.


Suggested change

/// Returns a map of rollup PR numbers to the set of member PR numbers that are part of that rollup.

/// Returns a map of rollup PR numbers to the set of member PR numbers that are part of that rollup.

/// Only returns non-closed rollup PRs.

Kobzol · 2026-01-12T10:20:04Z

src/database/operations.rs

+         AND pr.number = rm.rollup_pr_number
+        WHERE rm.repository = $1
+          AND pr.status IN ('open', 'draft')
+        ORDER BY rm.rollup_pr_number;


Suggested change

ORDER BY rm.rollup_pr_number;

Doesn't seem to be required, since we store the numbers in a hashmap/hashset anyway? Might help with cache locality for the hashmap, but that's not really needed here, and the DB won't have to sort the rollups.

src/templates.rs

Kobzol · 2026-01-12T10:25:18Z

web/templates/queue.html

+        tbody.addEventListener("mouseover", handler);
+        tbody.addEventListener("mouseleave", clear);
+
+        return () => {


It's nice that you implemented the event removal, but we don't call it anywhere anyway, so 🤷 I'd just remove it to shorten the code.

Will do. One related note: do you think it might be worth to support hovering on mobile devices too? I was thinking of hooking the hover handle to the touchstart event (and clear on touchend).

Hmm, not sure, because on click we select the PR - how do you actually distinguish hover and click on mobile? 🤔 If it can be done reliably, then might be useful, but I'd leave it for a future PR :)

Kobzol · 2026-01-12T10:25:37Z

web/templates/queue.html

+        };
+        const handler = (event) => {
+            // Reset highlight
+            clear()


Suggested change

clear()

clear();

Kobzol · 2026-01-12T10:25:45Z

web/templates/queue.html

+            }
+            let rollupPrMembers = rowElement.dataset.rollupPrMembers;
+            if (!rollupPrMembers) {
+                return


Suggested change

return

return;

Kobzol · 2026-01-13T20:03:33Z

src/github/rollup.rs

        .await
        .context("Cannot create PR")?;

+    db.register_rollup_members(


I was thinking about (many!) follow-up features that we could implement once this PR lands, and I realized that the problem with the missing PR id can be solved very easily. We can simply pre-insert the PR into the DB immediately here:

let pr_db = db.upsert_pull_request(..., pr.into());

and then use pr_db.id to record the rollup mapping in the DB.

Since we anyway have to do a JOIN with the PR table when selecting the rollup mapping due to filtering out open PRs, I think that storing the IDs in the mapping would be simpler, after all, would waste less DB space, and would provide proper FK mapping in the DB.

Could you please change it to store (FK to PR table, rollup PR number) in the rollup_member table? Sorry for the late notice 😅

That makes sense! Some questions:

I assume the fact we perform another upsert from the webhook event received by GitHub for the same PR doesn't matter, as it'd just write the same data. Adding a check to prevent writing PRs that are already in the table in the webhook logic seems pointless - is my assumption correct?

Wouldn't it make sense to change the member PR number in the same way? I assume we're guaranteed to have an entry in the PR table, as from what I know, rollups can only be created from the queue page, which implies PRs shown there have an entry in the table. Therefore we could use a FK for member PRs too for the same reasons. I think the only tradeoff is that writes would be slightly more expensive, but it shoudn't be a concern with the low amount of writes, and it might help future with future query patterns, if any.

I'm always looking for issues to contribute to (mostly during weekends, when i'm not too tired from working in a startup haha). If you want, write down the ideas in GitHub issues and I'll take care of them!

Yeah, the open PR webhook is not load-bearing. It can in theory race with the rollup handler, but that shouldn't matter much, Postgres will ensure atomicity of the PR creation.

A very good point! Yeah, I agree. In all cases where we will query this table, the corresponding PRs should already be in the DB. Including when we record the member entries.

Btw, after this change you can remove refresh_prs from rollup tests.

Okay, I will create issues for the features, once this PR lands.

Kobzol mentioned this pull request Dec 22, 2025

Filter out duplicate PRs when making a rollup #521

Merged

Kobzol reviewed Dec 22, 2025

View reviewed changes

Carbonhell force-pushed the feat/rollup-contents branch 3 times, most recently from 948ac1d to c1d42a4 Compare January 6, 2026 21:52

Carbonhell requested a review from Kobzol January 6, 2026 23:33

Kobzol reviewed Jan 7, 2026

View reviewed changes

src/github/rollup.rs Show resolved Hide resolved

src/github/rollup.rs Show resolved Hide resolved

Carbonhell added 2 commits January 11, 2026 23:37

feat(rollups): add rollup_contents database table, along with basic i…

41da37f

…nsert/get logic chore(git): add relevant gitattributes to ensure correct line endings for sqlx files

Carbonhell force-pushed the feat/rollup-contents branch from 41253eb to 4298378 Compare January 11, 2026 22:45

Carbonhell commented Jan 11, 2026

View reviewed changes

src/templates.rs Show resolved Hide resolved

Carbonhell added 2 commits January 11, 2026 23:56

feat(rollups): add visual cue in queue page to link rollup members wh…

d76bba4

…en hovering the row representing a rollup PR

fix(rollup,tests): simplify multiple_rollups_same_pr so that the goal…

0c5aeee

… is to verify the acceptable case of multiple rollups may point to the same member PR

Carbonhell force-pushed the feat/rollup-contents branch from 4298378 to 0c5aeee Compare January 11, 2026 22:58

Carbonhell requested a review from Kobzol January 11, 2026 23:01

Kobzol reviewed Jan 12, 2026

View reviewed changes

Kobzol reviewed Jan 13, 2026

View reviewed changes

	/// Returns a map of rollup PR numbers to the set of member PR numbers that are part of that rollup.
	/// Returns a map of rollup PR numbers to the set of member PR numbers that are part of that rollup.
	/// Only returns non-closed rollup PRs.

feat(rollups): add rollup_contents mapping #519

Are you sure you want to change the base?

feat(rollups): add rollup_contents mapping #519

Uh oh!

Conversation

Carbonhell commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kobzol commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kobzol left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Carbonhell commented Jan 6, 2026

Uh oh!

Kobzol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Kobzol commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Carbonhell commented Jan 7, 2026

Uh oh!

Kobzol commented Jan 8, 2026

Uh oh!

Uh oh!

Carbonhell commented Jan 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kobzol left a comment

Choose a reason for hiding this comment

Uh oh!

Kobzol Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Carbonhell Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Carbonhell commented Dec 22, 2025 •

edited

Loading

Kobzol commented Dec 22, 2025 •

edited

Loading

Kobzol left a comment •

edited

Loading

Kobzol commented Jan 7, 2026 •

edited

Loading

Carbonhell commented Jan 11, 2026 •

edited

Loading

Kobzol Jan 12, 2026 •

edited

Loading

Carbonhell Jan 13, 2026 •

edited

Loading