-
Notifications
You must be signed in to change notification settings - Fork 762
Cache newly created PRs when creating them in CLI #11913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
2a7c38b to
47451da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements caching for newly created pull requests when they are created via the CLI, ensuring they appear immediately in cache-only queries without requiring a separate sync operation.
Changes:
- Added a new
insertmethod toForgeReviewsHandleMutfor inserting or updating individual reviews in the database - Created a public
cache_reviewfunction inbut-forgeto cache a single review - Modified
publish_reviewto cache the newly created review immediately after creation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
crates/but-forge/src/lib.rs |
Exports the new cache_review function for public use |
crates/but-forge/src/db.rs |
Adds cache_review function that wraps the database insert operation with type conversion |
crates/but-db/src/table/forge_reviews.rs |
Implements insert method using SQLite's INSERT OR REPLACE for upsert functionality |
crates/but-db/tests/db/table/forge_review.rs |
Adds comprehensive tests for the new insert functionality (single insert, add to existing, update by number) |
crates/but-api/src/legacy/forge.rs |
Updates publish_review to cache the newly created review, with error handling that silently ignores failures |
b6dc151 to
fd13973
Compare
fd13973 to
bc5b9f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
bc5b9f0 to
27af9ee
Compare
| /// This is currently taking a project_id, there was some odd behaviour around the async code & the | ||
| /// need for the context to be ThreadSafe. I decided to defer looking into it more until we have | ||
| /// the time to properly delegificy this code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Context can't be held across await points. To do that, it needs to be turned into a thread-safe one.
Here is the change (I didn't want to push into this branch and cause confusion).
diff --git a/crates/but-api/src/legacy/forge.rs b/crates/but-api/src/legacy/forge.rs
index 263c8e22a0..8dcd288cfd 100644
--- a/crates/but-api/src/legacy/forge.rs
+++ b/crates/but-api/src/legacy/forge.rs
@@ -2,7 +2,7 @@
use anyhow::{Context as _, Result};
use but_api_macros::but_api;
use but_core::RepositoryExt;
-use but_ctx::Context;
+use but_ctx::{Context, ThreadSafeContext};
use but_forge::{
ForgeName, ReviewTemplateFunctions, available_review_templates, get_review_template_functions,
};
@@ -220,13 +220,13 @@ pub async fn publish_review(
project_id: ProjectId,
params: but_forge::CreateForgeReviewParams,
) -> Result<but_forge::ForgeReview> {
- let (base_branch, storage, project) = {
+ let (base_branch, storage, project, ctx) = {
let ctx = Context::new_from_legacy_project_id(project_id)?;
let base_branch = gitbutler_branch_actions::base::get_base_branch_data(&ctx)?;
let storage = but_forge_storage::Controller::from_path(but_path::app_data_dir()?);
let project = ctx.legacy_project.clone();
- (base_branch, storage, project)
+ (base_branch, storage, project, ctx.into_sync())
};
let review = but_forge::create_forge_review(
@@ -239,7 +239,7 @@ pub async fn publish_review(
)
.await?;
- if let Err(err) = try_cache_review(project_id, &review) {
+ if let Err(err) = try_cache_review(ctx, &review) {
tracing::warn!(?err, "Failed to cache newly created review");
};
@@ -248,11 +248,10 @@ pub async fn publish_review(
/// Try to cache a review.
///
-/// This is currently taking a project_id, there was some odd behaviour around the async code & the
-/// need for the context to be ThreadSafe. I decided to defer looking into it more until we have
-/// the time to properly delegificy this code.
-fn try_cache_review(project_id: ProjectId, review: &but_forge::ForgeReview) -> Result<()> {
- let mut ctx = Context::new_from_legacy_project_id(project_id)?;
+/// There was some odd behaviour around the async code & the
+/// need for the context to be ThreadSafe.
+fn try_cache_review(ctx: ThreadSafeContext, review: &but_forge::ForgeReview) -> Result<()> {
+ let mut ctx = ctx.into_thread_local();
let mut db = ctx.db.get_mut()?;
but_forge::cache_review(&mut db, review)?;
No description provided.