Skip to content
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
798bd5d
Initial plan
Copilot Feb 3, 2026
f0ea013
Add batching system for coordinate edits with database, processor, an…
Copilot Feb 3, 2026
75b2584
Switch from database batching to PR-based batching for better persist…
Copilot Feb 3, 2026
9afc276
Add lib.rs and make Edit fields public for batch processor access
Copilot Feb 3, 2026
f6b832e
Simplify to always auto-batch edits, remove BATCH_ENABLED setting and…
Copilot Feb 3, 2026
5dac6c6
Move AppData to lib.rs app module and fix all compilation errors
Copilot Feb 3, 2026
9ba60f5
Address code review feedback: fix typo, remove unused field, add erro…
Copilot Feb 3, 2026
279a1eb
Remove unnecessary infrastructure changes, keep minimal batching impl…
Copilot Feb 3, 2026
2e61cde
Refactor batch processor to use GitHub struct methods for clean archi…
Copilot Feb 3, 2026
3ac9cc4
Add PR description updates to batch processor - append each edit's de…
Copilot Feb 3, 2026
e0d31b6
Merge branch 'main' into copilot/add-batching-system-for-edits
CommanderStorm Feb 3, 2026
5b6cb05
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 3, 2026
d79b9f4
Fix critical bugs and apply review feedback: pagination, batch label,…
Copilot Feb 4, 2026
cdcc9f0
Merge branch 'main' into copilot/add-batching-system-for-edits
CommanderStorm Feb 4, 2026
f0cc3e8
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 4, 2026
357afee
Apply suggestions from code review
CommanderStorm Feb 4, 2026
3ba04ad
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 4, 2026
0eb9186
Apply suggestions from code review
CommanderStorm Feb 4, 2026
ad60348
Apply suggestion from @CommanderStorm
CommanderStorm Feb 4, 2026
2ab0719
Merge branch 'main' into copilot/add-batching-system-for-edits
CommanderStorm Feb 4, 2026
c07160b
Apply suggestions from code review
CommanderStorm Feb 4, 2026
a9c2264
Apply suggestions from code review
CommanderStorm Feb 4, 2026
0d72785
Apply suggestion from @CommanderStorm
CommanderStorm Feb 4, 2026
11f6976
Apply suggestion from @Copilot
CommanderStorm Feb 4, 2026
bb6bcc0
Apply suggestion from @CommanderStorm
CommanderStorm Feb 4, 2026
f2a846f
Merge branch 'main' into copilot/add-batching-system-for-edits
CommanderStorm Feb 5, 2026
3e736a6
Merge branch 'main' into copilot/add-batching-system-for-edits
CommanderStorm Feb 5, 2026
0f1c6bd
Merge branch 'main' into copilot/add-batching-system-for-edits
CommanderStorm Feb 11, 2026
953e48e
Fix compilation errors: make BATCH_LABEL public and fix incorrect log…
Copilot Feb 12, 2026
f078b09
Use extract_subject for first PR title to provide helpful context
Copilot Feb 12, 2026
258b8ee
Merge branch 'main' into copilot/add-batching-system-for-edits
CommanderStorm Mar 9, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 134 additions & 0 deletions server/src/external/github.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,140 @@ impl GitHub {
}
}
}

/// Find an open PR with a specific label
#[tracing::instrument]
pub async fn find_pr_with_label(self, label: &str) -> anyhow::Result<Option<(u64, String)>> {
let Some(octocrab) = self.octocrab else {
anyhow::bail!("GitHub client not initialized");
};

// Search through all pages of open PRs to find one with the label
let mut page = octocrab
.pulls("TUM-Dev", "NavigaTUM")
.list()
.state(octocrab::params::State::Open)
.per_page(100)
.send()
.await?;

loop {
for pr in &page.items {
if let Some(labels) = &pr.labels {
for pr_label in labels {
if pr_label.name == label {
return Ok(Some((pr.number, pr.head.ref_field.clone())));
}
}
}
}

// Check if there's a next page
match octocrab.get_page(&page.next).await? {
Some(next_page) => page = next_page,
None => break,
}
}

Ok(None)
}
Comment thread
CommanderStorm marked this conversation as resolved.

/// Update PR labels
#[tracing::instrument]
pub async fn update_pr_labels(self, pr_number: u64, labels: Vec<String>) -> anyhow::Result<()> {
let Some(octocrab) = self.octocrab else {
anyhow::bail!("GitHub client not initialized");
};

octocrab
.issues("TUM-Dev", "NavigaTUM")
.update(pr_number)
.labels(&labels)
.send()
.await?;

Ok(())
}

/// Update PR title
#[tracing::instrument]
pub async fn update_pr_title(self, pr_number: u64, title: &str) -> anyhow::Result<()> {
let Some(octocrab) = self.octocrab else {
anyhow::bail!("GitHub client not initialized");
};

octocrab
.issues("TUM-Dev", "NavigaTUM")
.update(pr_number)
.title(title)
.send()
.await?;

Ok(())
}

/// Get the number of commits in a PR
#[tracing::instrument]
pub async fn get_pr_commit_count(self, pr_number: u64) -> anyhow::Result<usize> {
let Some(octocrab) = self.octocrab else {
anyhow::bail!("GitHub client not initialized");
};

// Fetch the first page of commits (up to 100 per page)
let mut page = octocrab
.pulls("TUM-Dev", "NavigaTUM")
.pr_commits(pr_number)
.per_page(100)
.send()
.await?;

// Count commits from the first page
let mut total_commits = page.items.len();

// Follow pagination links to count commits from all subsequent pages
while let Some(next_page) = octocrab.get_page(&page.next).await? {
total_commits += next_page.items.len();
page = next_page;
}

Ok(total_commits)
}

/// Get PR description (body)
#[tracing::instrument]
pub async fn get_pr_description(self, pr_number: u64) -> anyhow::Result<String> {
let Some(octocrab) = self.octocrab else {
anyhow::bail!("GitHub client not initialized");
};

let pr = octocrab
.pulls("TUM-Dev", "NavigaTUM")
.get(pr_number)
.await?;

Ok(pr.body.unwrap_or_default())
}

/// Update PR description (body)
#[tracing::instrument(skip(description))]
pub async fn update_pr_description(
self,
pr_number: u64,
description: &str,
) -> anyhow::Result<()> {
let Some(octocrab) = self.octocrab else {
anyhow::bail!("GitHub client not initialized");
};

octocrab
.issues("TUM-Dev", "NavigaTUM")
.update(pr_number)
.body(description)
.send()
.await?;

Ok(())
}
}

#[cfg(test)]
Expand Down
95 changes: 95 additions & 0 deletions server/src/routes/feedback/batch_processor/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
use tracing::{error, info};

use crate::external::github::GitHub;

const BATCH_LABEL: &str = "batch-in-progress";

/// Find the current open batch PR (if any)
pub async fn find_open_batch_pr() -> anyhow::Result<Option<(u64, String)>> {
let github = GitHub::default();

match github.find_pr_with_label(BATCH_LABEL).await {
Ok(Some((pr_number, branch))) => {
info!(error=e?, %pr_number, "Found open batch PR");
Comment thread
CommanderStorm marked this conversation as resolved.
Outdated
Ok(Some((pr_number, branch)))
}
Ok(None) => {
info!("No open batch PR found");
Ok(None)
}
Err(e) => {
error!(error=?e, "Error finding batch PR");
Err(e)
}
}
}

/// Update batch PR metadata (title with edit count, labels, and description)
pub async fn update_batch_pr_metadata(
pr_number: u64,
edit_request: &super::proposed_edits::EditRequest,
new_edit_description: &str,
) -> anyhow::Result<()> {
// Update labels based on edit type
let mut labels = vec![BATCH_LABEL.to_string(), "webform".to_string()];

// Check edit types using the extract_labels method
let edit_labels = edit_request.extract_labels();
for label in edit_labels {
if label != "webform" && !labels.contains(&label) {
labels.push(label);
}
}

let github = GitHub::default();
match github.update_pr_labels(pr_number, labels).await {
Ok(_) => info!("Updated labels for batch PR #{}", pr_number),
Err(e) => error!(
error=?e, %pr_number, "Failed to update labels for batch PR"
),
}

// Get commits to count edits
let github = GitHub::default();
let edit_count = match github.get_pr_commit_count(pr_number).await {
Ok(count) => count,
Err(e) => {
error!(error=?e, %pr_number, "Failed to get commit count for PR");
return Err(e);
}
};
Comment on lines +54 to +60
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's another race condition: if two requests commit to the same batch PR simultaneously, the commit count retrieved at line 54 might not reflect the latest commit from the concurrent request, leading to incorrect edit numbering in the PR description (e.g., both edits labeled as "Edit #2").

Copilot uses AI. Check for mistakes.

// Update PR title with edit count
let github = GitHub::default();
let title = format!("chore(data): batch coordinate edits ({edit_count} edits)");
Comment on lines +54 to +64
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The edit count is based on commit count, which means if the first commit creates the PR, the count starts at 1 (correct). However, the logic assumes each edit results in exactly one commit. If apply_changes_and_generate_description creates multiple commits or no commits in edge cases, the edit numbering will be incorrect. This tight coupling between commits and edits is fragile.

Copilot uses AI. Check for mistakes.
match github.update_pr_title(pr_number, &title).await {
Ok(_) => info!(%pr_number, "Updated title for batch PR"),
Err(e) => error!(error=?e, %pr_number, "Failed to update title for batch PR"),
}
Comment on lines +45 to +68
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function logs errors for label and title update failures but doesn't return early, continuing to execute the remaining operations. This could lead to an inconsistent state where some metadata is updated but not others. Consider either returning early on errors or collecting all errors and handling them together at the end.

Copilot uses AI. Check for mistakes.

// Append to PR description
let github = GitHub::default();
let current_description = github
.get_pr_description(pr_number)
.await
.unwrap_or_default();

// Append the new edit's description
let updated_description = if current_description.is_empty() {
format!("## Batched Coordinate Edits\n\n### Edit #{edit_count}\n{new_edit_description}")
} else {
format!("{current_description}\n\n---\n\n### Edit #{edit_count}\n{new_edit_description}")
};

let github = GitHub::default();
Comment thread
CommanderStorm marked this conversation as resolved.
match github
.update_pr_description(pr_number, &updated_description)
.await
{
Ok(_) => info!(%pr_number, "Updated description for batch PR"),
Err(e) => error!(error=?e, %pr_number,
"Failed to update description for batch PR"),
}

Ok(())
}
Comment on lines +1 to +95
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no tests for the new batch processor functionality. Looking at the codebase, most modules in server/src/routes/feedback/proposed_edits/ have test coverage (coordinate.rs, description.rs, image.rs, tmp_repo.rs all have #[cfg(test)] sections).

The batch processor should have unit tests covering:

  • Finding batch PRs with the label
  • Handling the case when no batch PR exists
  • Updating PR metadata (labels, title, description)
  • Label deduplication logic
  • Description formatting with edit numbers

This is particularly important given the critical bugs found in the implementation, which could have been caught by tests.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +95
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The batch_processor module lacks test coverage. Other modules in the proposed_edits directory (coordinate.rs, description.rs, image.rs, tmp_repo.rs) all include #[cfg(test)] sections with tests. The batch_processor contains critical business logic for finding batch PRs, updating metadata, and managing labels, but has no tests to verify this behavior works correctly.

Copilot uses AI. Check for mistakes.
1 change: 1 addition & 0 deletions server/src/routes/feedback/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
pub mod batch_processor;
pub mod post_feedback;
pub mod proposed_edits;
pub mod tokens;
57 changes: 46 additions & 11 deletions server/src/routes/feedback/proposed_edits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::path::Path;
use actix_web::web::{Data, Json};
use actix_web::{HttpResponse, post};
use serde::Deserialize;
use tracing::error;
use tracing::{error, info};
#[expect(
unused_imports,
reason = "has to be imported as otherwise utoipa generates incorrect code"
Expand Down Expand Up @@ -79,7 +79,7 @@ impl EditRequest {
.collect()
}

fn extract_labels(&self) -> Vec<String> {
pub(super) fn extract_labels(&self) -> Vec<String> {
let mut labels = vec!["webform".to_string()];

if self
Expand All @@ -95,6 +95,7 @@ impl EditRequest {
}
labels
}
Comment thread
CommanderStorm marked this conversation as resolved.

fn extract_subject(&self) -> String {
use itertools::Itertools;
let coordinate_edits = self.edits_for(|edit| edit.coordinate);
Expand Down Expand Up @@ -178,22 +179,56 @@ pub async fn propose_edits(
};

let branch_name = format!("usergenerated/request-{}", rand::random::<u16>());

// Try to find an open batch PR and use it
let batch_pr = super::batch_processor::find_open_batch_pr()
.await
.ok()
.flatten();

let (branch_to_use, pr_number_opt) = match batch_pr {
Some((pr_number, batch_branch)) => {
info!(%pr_number, "Adding edit to existing batch PR");
(batch_branch, Some(pr_number))
}
None => (branch_name, None),
};
Comment on lines +189 to +195
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no check to verify that the found batch PR's branch is writable or that it's not already merged/closed before attempting to commit to it. If the batch PR is found but the branch has been deleted or protected, the apply_changes_and_generate_description call will fail. Consider adding a check to verify the PR state and branch availability before using it.

Copilot uses AI. Check for mistakes.

match req_data
.apply_changes_and_generate_description(&branch_name)
.apply_changes_and_generate_description(&branch_to_use)
.await
{
Ok(description) => {
GitHub::default()
.open_pr(
branch_name,
&format!(
"chore(data): {subject}",
subject = req_data.extract_subject()
),
if let Some(pr_number) = pr_number_opt {
// Update metadata for batch PR (including appending description)
if let Err(e) = super::batch_processor::update_batch_pr_metadata(
pr_number,
&req_data,
&description,
req_data.extract_labels(),
)
.await
{
error!(error = e?, "Failed to update batch PR metadata");
Comment thread
CommanderStorm marked this conversation as resolved.
Outdated
}

let pr_url = format!("https://github.com/TUM-Dev/NavigaTUM/pull/{pr_number}");
HttpResponse::Created()
.content_type("text/plain")
.body(pr_url)
Comment thread
CommanderStorm marked this conversation as resolved.
} else {
// Create new batch PR with batch-in-progress label
let mut labels = req_data.extract_labels();
labels.push("batch-in-progress".to_string());
Comment thread
CommanderStorm marked this conversation as resolved.
Outdated

GitHub::default()
.open_pr(
branch_to_use,
"chore(data): batch coordinate edits (1 edit)",
&format!("## Batched Coordinate Edits\n\n### Edit #1\n{description}"),
labels,
)
.await
}
Comment thread
CommanderStorm marked this conversation as resolved.
Comment thread
CommanderStorm marked this conversation as resolved.
}
Err(error) => {
error!(?error, "could not apply changes");
Expand Down
Loading