From 422451528827bf6962ea4e4b7d603519210d3191 Mon Sep 17 00:00:00 2001 From: bytedream Date: Sun, 23 Nov 2025 16:58:51 +0100 Subject: [PATCH 1/3] Fix incorrect viewed files counter if file has changed --- services/gitdiff/gitdiff.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 4ad06bc04ff67..2830287f690e3 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1434,15 +1434,19 @@ outer: } } - // Explicitly store files that have changed in the database, if any is present at all. - // This has the benefit that the "Has Changed" attribute will be present as long as the user does not explicitly mark this file as viewed, so it will even survive a page reload after marking another file as viewed. - // On the other hand, this means that even if a commit reverting an unseen change is committed, the file will still be seen as changed. if len(filesChangedSinceLastDiff) > 0 { + // Explicitly store files that have changed in the database, if any is present at all. + // This has the benefit that the "Has Changed" attribute will be present as long as the user does not explicitly mark this file as viewed, so it will even survive a page reload after marking another file as viewed. + // On the other hand, this means that even if a commit reverting an unseen change is committed, the file will still be seen as changed. err := pull_model.UpdateReviewState(ctx, review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff) if err != nil { log.Warn("Could not update review for user %d, pull %d, commit %s and the changed files %v: %v", review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff, err) return nil, err } + // Additionally, update the changed files state locally to reflect the changes immediately + for filename, state := range filesChangedSinceLastDiff { + review.UpdatedFiles[filename] = state + } } return review, nil From 215620edd60c321553398c419fbfc44d35b7c0c9 Mon Sep 17 00:00:00 2001 From: bytedream Date: Sun, 23 Nov 2025 17:15:53 +0100 Subject: [PATCH 2/3] Make linter happy --- services/gitdiff/gitdiff.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 2830287f690e3..529995df28f77 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -12,6 +12,7 @@ import ( "html" "html/template" "io" + "maps" "net/url" "path" "sort" @@ -1444,9 +1445,7 @@ outer: return nil, err } // Additionally, update the changed files state locally to reflect the changes immediately - for filename, state := range filesChangedSinceLastDiff { - review.UpdatedFiles[filename] = state - } + maps.Copy(review.UpdatedFiles, filesChangedSinceLastDiff) } return review, nil From 8a3ff4bc7a90378019f4d8dfc629d0e05a1e36c2 Mon Sep 17 00:00:00 2001 From: bytedream Date: Mon, 24 Nov 2025 00:54:49 +0100 Subject: [PATCH 3/3] Use alternative method to save modified changed files state --- models/pull/review_state.go | 10 +++++----- routers/web/repo/pull_review.go | 2 +- services/gitdiff/gitdiff.go | 7 +++---- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/models/pull/review_state.go b/models/pull/review_state.go index e8b759c0cc62e..1fd416fc3f97a 100644 --- a/models/pull/review_state.go +++ b/models/pull/review_state.go @@ -73,18 +73,18 @@ func GetReviewState(ctx context.Context, userID, pullID int64, commitSHA string) // UpdateReviewState updates the given review inside the database, regardless of whether it existed before or not // The given map of files with their viewed state will be merged with the previous review, if present -func UpdateReviewState(ctx context.Context, userID, pullID int64, commitSHA string, updatedFiles map[string]ViewedState) error { +func UpdateReviewState(ctx context.Context, userID, pullID int64, commitSHA string, updatedFiles map[string]ViewedState) (*ReviewState, error) { log.Trace("Updating review for user %d, repo %d, commit %s with the updated files %v.", userID, pullID, commitSHA, updatedFiles) review, exists, err := GetReviewState(ctx, userID, pullID, commitSHA) if err != nil { - return err + return nil, err } if exists { review.UpdatedFiles = mergeFiles(review.UpdatedFiles, updatedFiles) } else if previousReview, err := getNewestReviewStateApartFrom(ctx, userID, pullID, commitSHA); err != nil { - return err + return nil, err // Overwrite the viewed files of the previous review if present } else if previousReview != nil { @@ -98,11 +98,11 @@ func UpdateReviewState(ctx context.Context, userID, pullID int64, commitSHA stri if !exists { log.Trace("Inserting new review for user %d, repo %d, commit %s with the updated files %v.", userID, pullID, commitSHA, review.UpdatedFiles) _, err := engine.Insert(review) - return err + return nil, err } log.Trace("Updating already existing review with ID %d (user %d, repo %d, commit %s) with the updated files %v.", review.ID, userID, pullID, commitSHA, review.UpdatedFiles) _, err = engine.ID(review.ID).Update(&ReviewState{UpdatedFiles: review.UpdatedFiles}) - return err + return review, err } // mergeFiles merges the given maps of files with their viewing state into one map. diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 18e14e9b224c4..f064058221ea6 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -331,7 +331,7 @@ func UpdateViewedFiles(ctx *context.Context) { updatedFiles[file] = state } - if err := pull_model.UpdateReviewState(ctx, ctx.Doer.ID, pull.ID, data.HeadCommitSHA, updatedFiles); err != nil { + if _, err := pull_model.UpdateReviewState(ctx, ctx.Doer.ID, pull.ID, data.HeadCommitSHA, updatedFiles); err != nil { ctx.ServerError("UpdateReview", err) } } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 529995df28f77..6e15f7160956c 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -12,7 +12,6 @@ import ( "html" "html/template" "io" - "maps" "net/url" "path" "sort" @@ -1439,13 +1438,13 @@ outer: // Explicitly store files that have changed in the database, if any is present at all. // This has the benefit that the "Has Changed" attribute will be present as long as the user does not explicitly mark this file as viewed, so it will even survive a page reload after marking another file as viewed. // On the other hand, this means that even if a commit reverting an unseen change is committed, the file will still be seen as changed. - err := pull_model.UpdateReviewState(ctx, review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff) + updatedReview, err := pull_model.UpdateReviewState(ctx, review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff) if err != nil { log.Warn("Could not update review for user %d, pull %d, commit %s and the changed files %v: %v", review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff, err) return nil, err } - // Additionally, update the changed files state locally to reflect the changes immediately - maps.Copy(review.UpdatedFiles, filesChangedSinceLastDiff) + // Update the local review to reflect the changes immediately + review = updatedReview } return review, nil