From 44057ab0f7e77b934e5807aa60c88ded9985fb6b Mon Sep 17 00:00:00 2001 From: Houssem Eddine BENABDALLAH Date: Thu, 29 Oct 2026 11:36:35 +0100 Subject: [PATCH 1/7] feat: let Tom spot merge conflicts in the replies and readers pick a side --- KNOWLEDGE_BASE.md | 9 + PRODUCT.md | 2 +- app/Actions/CreateCommit.php | 2 + app/Actions/CreateMergeConflict.php | 101 ++++++++ app/Actions/CreateMergeConflictVote.php | 27 +++ app/Actions/UpdateCommitStatuses.php | 2 + app/Ai/Agents/ConflictSpotter.php | 55 +++++ app/Enums/Feature.php | 1 + app/Enums/MergeConflictSide.php | 14 ++ app/Http/Controllers/CommitController.php | 4 +- .../MergeConflictVoteController.php | 25 ++ .../CreateMergeConflictVoteRequest.php | 65 +++++ app/Jobs/SpotMergeConflict.php | 58 +++++ app/Models/Commit.php | 9 + app/Models/MergeConflict.php | 109 +++++++++ app/Models/MergeConflictVote.php | 59 +++++ app/Queries/MergeConflictQuery.php | 84 +++++++ database/factories/MergeConflictFactory.php | 33 +++ .../factories/MergeConflictVoteFactory.php | 29 +++ ...29_090000_create_merge_conflicts_table.php | 33 +++ resources/js/components/commit-card.tsx | 18 +- resources/js/components/commit-poll.tsx | 2 +- resources/js/components/merge-conflict.tsx | 224 ++++++++++++++++++ resources/js/pages/commit/show.tsx | 4 + resources/js/pages/operations/features.tsx | 5 + resources/js/types/commit.ts | 17 ++ resources/js/types/operations.ts | 2 +- routes/web.php | 3 + .../Controllers/FeatureControllerTest.php | 6 +- .../MergeConflictVoteControllerTest.php | 116 +++++++++ .../Unit/Actions/CreateMergeConflictTest.php | 129 ++++++++++ .../Unit/Actions/UpdateCommitStatusesTest.php | 3 +- tests/Unit/Jobs/SpotMergeConflictTest.php | 40 ++++ tests/Unit/MergeConflict.test.tsx | 126 ++++++++++ 34 files changed, 1407 insertions(+), 9 deletions(-) create mode 100644 app/Actions/CreateMergeConflict.php create mode 100644 app/Actions/CreateMergeConflictVote.php create mode 100644 app/Ai/Agents/ConflictSpotter.php create mode 100644 app/Enums/MergeConflictSide.php create mode 100644 app/Http/Controllers/MergeConflictVoteController.php create mode 100644 app/Http/Requests/CreateMergeConflictVoteRequest.php create mode 100644 app/Jobs/SpotMergeConflict.php create mode 100644 app/Models/MergeConflict.php create mode 100644 app/Models/MergeConflictVote.php create mode 100644 app/Queries/MergeConflictQuery.php create mode 100644 database/factories/MergeConflictFactory.php create mode 100644 database/factories/MergeConflictVoteFactory.php create mode 100644 database/migrations/2026_10_29_090000_create_merge_conflicts_table.php create mode 100644 resources/js/components/merge-conflict.tsx create mode 100644 tests/Feature/Controllers/MergeConflictVoteControllerTest.php create mode 100644 tests/Unit/Actions/CreateMergeConflictTest.php create mode 100644 tests/Unit/Jobs/SpotMergeConflictTest.php create mode 100644 tests/Unit/MergeConflict.test.tsx diff --git a/KNOWLEDGE_BASE.md b/KNOWLEDGE_BASE.md index 831a262a4..697dfd989 100644 --- a/KNOWLEDGE_BASE.md +++ b/KNOWLEDGE_BASE.md @@ -28,6 +28,7 @@ This file documents the platform's features, stack, and stories. Feature entries - [Replies and threads](#replies-and-threads) - [Quoting commits](#quoting-commits) - [Polls and voting](#polls-and-voting) + - [Merge conflicts](#merge-conflicts) - [Scheduled commits](#scheduled-commits) - [Deleting commits and undo](#deleting-commits-and-undo) - [Stars](#stars) @@ -324,6 +325,14 @@ Signed-in readers can choose one option and vote once while an approved, visible Sources: [poll editor](resources/js/components/poll-editor.tsx), [poll results](app/Models/Poll.php), [voting rules](app/Rules/OpenPoll.php), [approval and poll opening](app/Actions/UpdateCommitStatuses.php). +### Merge conflicts + +When two people take opposite sides in the direct replies to a commit, Tom can open a **merge conflict** on it. A few minutes after a reply is approved, Tom reads the first 30 replies, leaving out his own. Only a clear disagreement between two different authors counts. Tom reads a thread at most once an hour, and a commit gets at most one merge conflict. + +The two replies are shown on the commit's page between git conflict markers. For **24 hours**, signed-in readers who can see both replies can accept one side. Picking a side is final, and the two authors cannot vote. Counts are visible to those who picked a side and to the two authors, and to everyone once voting closes. The side with more votes is then shown as kept. The conflict is hidden from readers who cannot see one of the replies, for example across a block or a mute. Admins can switch merge conflicts off from the operations dashboard, which also hides those already opened. + +Sources: [spotting a conflict](app/Actions/CreateMergeConflict.php), [Tom's instructions](app/Ai/Agents/ConflictSpotter.php), [what readers see](app/Queries/MergeConflictQuery.php), [conflict view](resources/js/components/merge-conflict.tsx). + ### Scheduled commits Use the composer's scheduling control to submit a top-level commit later. Pick a future time within **30 days**, in five-minute steps; you can have at most **10 outstanding scheduled commits**. Scheduled posts can carry images or a poll under the normal attachment rules, but replies and quotes cannot be scheduled. Scheduling counts against the 10-per-minute submission limit. The scheduled-commits page, linked from the scheduling control, is private to you: it lists queued posts, which you can cancel, and already submitted ones with their outcome. Members of an organization can schedule a commit as the organization, by picking it from their avatar first; the page shows which ones go out as an organization. diff --git a/PRODUCT.md b/PRODUCT.md index 13dbe7437..e41efce98 100644 --- a/PRODUCT.md +++ b/PRODUCT.md @@ -31,7 +31,7 @@ Developers who left X or Bluesky, or are thinking about it, and want a feed with What exists, one line each (details in the knowledge base): -- **Commits:** text with code, images, link cards and embedded players, or a poll; replies, quotes, threads, scheduling, translation. +- **Commits:** text with code, images, link cards and embedded players, or a poll; replies, quotes, threads, scheduling, translation, and merge conflicts where Tom spots two replies that disagree and readers pick a side. - **Logs:** Following, Latest, and Trending. - **Engagement:** follows, stars, saved commits, pins, mentions, and pastéis (an opt-in daily allowance of pastries). - **Profiles:** mirrored from GitHub, with an activity graph, streaks, achievements, a Unix rank, and a live marker while you stream on YouTube. diff --git a/app/Actions/CreateCommit.php b/app/Actions/CreateCommit.php index 9bc21f6ba..29bb67be5 100644 --- a/app/Actions/CreateCommit.php +++ b/app/Actions/CreateCommit.php @@ -13,6 +13,7 @@ use App\Jobs\ModerateCommit; use App\Jobs\ProcessCommitHeadlines; use App\Jobs\ReplyAsTom; +use App\Jobs\SpotMergeConflict; use App\Jobs\UnfurlLink; use App\Models\Commit; use App\Models\LinkPreview; @@ -121,6 +122,7 @@ public function handle(User $user, string $body, ?Commit $parent = null, ?Commit if ($commit->status === CommitStatus::Approved && ! $organization instanceof Organization) { dispatch(new CheckUserLevel($user))->afterCommit(); + SpotMergeConflict::dispatchFor($commit); } LinkCommitTopics::dispatchFor($commit); diff --git a/app/Actions/CreateMergeConflict.php b/app/Actions/CreateMergeConflict.php new file mode 100644 index 000000000..8b9d921f5 --- /dev/null +++ b/app/Actions/CreateMergeConflict.php @@ -0,0 +1,101 @@ +features->enabled(Feature::MergeConflicts) + || $commit->status !== CommitStatus::Approved + || $commit->mergeConflict()->exists()) { + return null; + } + + $replies = $this->replies($commit); + + if ($replies->pluck('user_id')->unique()->count() < 2 + || ! Cache::add("merge-conflict-read:{$commit->id}", true, self::ReadEvery)) { + return null; + } + + $response = new ConflictSpotter()->prompt(collect([ + 'post' => mb_substr($commit->body, 0, self::MaxReplyLength), + 'replies' => $replies->values()->map(fn (Commit $reply, int $index): array => [ + 'number' => $index + 1, + 'author' => '@'.$reply->user?->handle, + 'body' => mb_substr($reply->body, 0, self::MaxReplyLength), + ]), + ])->toJson()); + + $ours = $replies->values()->get(is_int($response['ours'] ?? null) ? $response['ours'] - 1 : -1); + $theirs = $replies->values()->get(is_int($response['theirs'] ?? null) ? $response['theirs'] - 1 : -1); + + if (($response['conflict'] ?? false) !== true + || ! $ours instanceof Commit || ! $theirs instanceof Commit + || $ours->user_id === $theirs->user_id) { + return null; + } + + [$ours, $theirs] = $ours->created_at->lessThanOrEqualTo($theirs->created_at) ? [$ours, $theirs] : [$theirs, $ours]; + + return MergeConflict::query()->createOrFirst(['commit_id' => $commit->id], [ + 'ours_id' => $ours->id, + 'theirs_id' => $theirs->id, + 'closes_at' => now()->addHours(MergeConflict::VotingHours), + ]); + } + + /** + * @return Collection + */ + private function replies(Commit $commit): Collection + { + $tom = $this->tomQuery->get(); + + return $commit->replies() + ->where('status', CommitStatus::Approved) + ->whereNotNull('user_id') + ->where('body', '!=', '') + ->when($tom instanceof User, fn ($query) => $query->where('user_id', '!=', $tom?->id)) + ->with('user') + ->oldest() + ->oldest('id') + ->limit(self::MaxReplies) + ->get(); + } +} diff --git a/app/Actions/CreateMergeConflictVote.php b/app/Actions/CreateMergeConflictVote.php new file mode 100644 index 000000000..093ca4980 --- /dev/null +++ b/app/Actions/CreateMergeConflictVote.php @@ -0,0 +1,27 @@ +insertOrIgnore([ + 'merge_conflict_id' => $mergeConflict->id, + 'user_id' => $user->id, + 'side' => $side->value, + 'created_at' => now(), + 'updated_at' => now(), + ]) === 1; + } +} diff --git a/app/Actions/UpdateCommitStatuses.php b/app/Actions/UpdateCommitStatuses.php index a7c7ddf67..2d174338d 100644 --- a/app/Actions/UpdateCommitStatuses.php +++ b/app/Actions/UpdateCommitStatuses.php @@ -10,6 +10,7 @@ use App\Jobs\LinkCommitTopics; use App\Jobs\ProcessCommitHeadlines; use App\Jobs\ReplyAsTom; +use App\Jobs\SpotMergeConflict; use App\Models\Commit; use App\Models\Headline; use App\Models\Notification; @@ -116,6 +117,7 @@ public function handle(array $commitIds, CommitStatus $status, ?string $reason = $commits->each(fn (Commit $commit) => $this->createWorldReply->handle($commit)); LinkCommitTopics::dispatchFor(...$commits->all()); ProcessCommitHeadlines::dispatchFor(...$commits->all()); + SpotMergeConflict::dispatchFor(...$commits->all()); if (($tom = $this->tomQuery->get()) instanceof User) { foreach ($commits as $commit) { diff --git a/app/Ai/Agents/ConflictSpotter.php b/app/Ai/Agents/ConflictSpotter.php new file mode 100644 index 000000000..93741c2a5 --- /dev/null +++ b/app/Ai/Agents/ConflictSpotter.php @@ -0,0 +1,55 @@ + */ + public function schema(JsonSchema $schema): array + { + return [ + 'conflict' => $schema->boolean()->required(), + 'ours' => $schema->integer()->required(), + 'theirs' => $schema->integer()->required(), + ]; + } + + /** @return array */ + public function providerOptions(Lab|string $provider): array + { + return [ + 'thinking' => ['type' => 'disabled'], + 'response_format' => ['type' => 'json_object'], + ]; + } +} diff --git a/app/Enums/Feature.php b/app/Enums/Feature.php index 70485a003..6e6f1cee4 100644 --- a/app/Enums/Feature.php +++ b/app/Enums/Feature.php @@ -11,6 +11,7 @@ enum Feature: string { case Translations = 'translations'; case OrganizationCommits = 'organization_commits'; + case MergeConflicts = 'merge_conflicts'; /** * Features are on until switched off, except one still being built, which stays off until switched on. diff --git a/app/Enums/MergeConflictSide.php b/app/Enums/MergeConflictSide.php new file mode 100644 index 000000000..50763ad25 --- /dev/null +++ b/app/Enums/MergeConflictSide.php @@ -0,0 +1,14 @@ + fn (): Collection => $ancestors->get($commit, $currentUser), 'continuation' => $getContinuation, 'replies' => fn (): ?CursorPaginator => $getContinuation()->commits->isEmpty() ? $getReplies() : null, + 'mergeConflict' => fn (): ?array => $mergeConflicts->get($commit, $currentUser), ])->withViewData(['preview' => $preview]); } diff --git a/app/Http/Controllers/MergeConflictVoteController.php b/app/Http/Controllers/MergeConflictVoteController.php new file mode 100644 index 000000000..c856e37ab --- /dev/null +++ b/app/Http/Controllers/MergeConflictVoteController.php @@ -0,0 +1,25 @@ +handle($user, $request->mergeConflict(), $request->side()); + + return response()->json([ + 'mergeConflict' => $mergeConflicts->get($commit, $user), + ]); + } +} diff --git a/app/Http/Requests/CreateMergeConflictVoteRequest.php b/app/Http/Requests/CreateMergeConflictVoteRequest.php new file mode 100644 index 000000000..b0c645764 --- /dev/null +++ b/app/Http/Requests/CreateMergeConflictVoteRequest.php @@ -0,0 +1,65 @@ +commit()->mergeConflict; + + return $this->commit()->isVisibleTo($this->voter()) + && $mergeConflict instanceof MergeConflict + && $mergeConflict->canBeVotedOnBy($this->voter()); + } + + /** + * @return array> + */ + public function rules(): array + { + return [ + 'side' => ['required', Rule::enum(MergeConflictSide::class)], + ]; + } + + public function side(): MergeConflictSide + { + return $this->enum('side', MergeConflictSide::class) ?? MergeConflictSide::Ours; + } + + public function mergeConflict(): MergeConflict + { + $mergeConflict = $this->commit()->mergeConflict; + assert($mergeConflict instanceof MergeConflict); + + return $mergeConflict; + } + + private function commit(): Commit + { + $commit = $this->route('commit'); + assert($commit instanceof Commit); + + return $commit; + } + + private function voter(): User + { + $user = $this->user(); + assert($user instanceof User); + + return $user; + } +} diff --git a/app/Jobs/SpotMergeConflict.php b/app/Jobs/SpotMergeConflict.php new file mode 100644 index 000000000..62290395c --- /dev/null +++ b/app/Jobs/SpotMergeConflict.php @@ -0,0 +1,58 @@ + $reply->parent_id, $replies))); + + foreach ($parentIds as $parentId) { + dispatch(new self($parentId))->delay(now()->addMinutes(5))->afterCommit(); + } + } + + public function uniqueId(): string + { + return $this->commitId; + } + + public function handle(CreateMergeConflict $action): void + { + $commit = Commit::query()->find($this->commitId); + + if ($commit instanceof Commit) { + $action->handle($commit); + } + } +} diff --git a/app/Models/Commit.php b/app/Models/Commit.php index 805af50de..ec15f8861 100644 --- a/app/Models/Commit.php +++ b/app/Models/Commit.php @@ -56,6 +56,7 @@ * @property-read string|null $continuation_id * @property-read LinkPreview|null $linkPreview * @property-read Poll|null $poll + * @property-read MergeConflict|null $mergeConflict * @property-read Commit|null $quotedCommit * @property-read Collection $quotes * @property-read Collection $images @@ -311,6 +312,14 @@ public function poll(): HasOne return $this->hasOne(Poll::class); } + /** + * @return HasOne + */ + public function mergeConflict(): HasOne + { + return $this->hasOne(MergeConflict::class); + } + /** * @return BelongsTo */ diff --git a/app/Models/MergeConflict.php b/app/Models/MergeConflict.php new file mode 100644 index 000000000..f6464b379 --- /dev/null +++ b/app/Models/MergeConflict.php @@ -0,0 +1,109 @@ + $votes + */ +final class MergeConflict extends Model +{ + /** @use HasFactory */ + use HasFactory; + + use HasUuids; + + /** + * How long readers have to pick a side. + */ + public const int VotingHours = 24; + + /** + * @return array + */ + public function casts(): array + { + return [ + 'id' => 'string', + 'commit_id' => 'string', + 'ours_id' => 'string', + 'theirs_id' => 'string', + 'closes_at' => 'datetime', + 'created_at' => 'datetime', + 'updated_at' => 'datetime', + ]; + } + + /** + * @return BelongsTo + */ + public function commit(): BelongsTo + { + return $this->belongsTo(Commit::class); + } + + /** + * @return BelongsTo + */ + public function ours(): BelongsTo + { + return $this->belongsTo(Commit::class, 'ours_id'); + } + + /** + * @return BelongsTo + */ + public function theirs(): BelongsTo + { + return $this->belongsTo(Commit::class, 'theirs_id'); + } + + /** + * @return HasMany + */ + public function votes(): HasMany + { + return $this->hasMany(MergeConflictVote::class); + } + + public function isClosed(): bool + { + return now()->greaterThanOrEqualTo($this->closes_at); + } + + /** + * The two authors are a side each, so they don't get a vote. + */ + public function canBeVotedOnBy(User $user): bool + { + return ! $this->isClosed() && ! in_array($user->id, [$this->ours->user_id, $this->theirs->user_id], true); + } + + public function sideOf(User $user): ?MergeConflictSide + { + return $this->votes()->whereBelongsTo($user)->first()?->side; + } +} diff --git a/app/Models/MergeConflictVote.php b/app/Models/MergeConflictVote.php new file mode 100644 index 000000000..f26edd54d --- /dev/null +++ b/app/Models/MergeConflictVote.php @@ -0,0 +1,59 @@ + */ + use HasFactory; + + /** + * @return array + */ + public function casts(): array + { + return [ + 'id' => 'integer', + 'merge_conflict_id' => 'string', + 'user_id' => 'string', + 'side' => MergeConflictSide::class, + 'created_at' => 'datetime', + 'updated_at' => 'datetime', + ]; + } + + /** + * @return BelongsTo + */ + public function mergeConflict(): BelongsTo + { + return $this->belongsTo(MergeConflict::class); + } + + /** + * @return BelongsTo + */ + public function user(): BelongsTo + { + return $this->belongsTo(User::class); + } +} diff --git a/app/Queries/MergeConflictQuery.php b/app/Queries/MergeConflictQuery.php new file mode 100644 index 000000000..0ad8852cd --- /dev/null +++ b/app/Queries/MergeConflictQuery.php @@ -0,0 +1,84 @@ +features->enabled(Feature::MergeConflicts) ? $commit->mergeConflict()->first() : null; + + if (! $mergeConflict instanceof MergeConflict) { + return null; + } + + $sides = Commit::query() + ->visibleTo($viewer) + ->withoutMutedAuthorsFor($viewer) + ->whereKey([$mergeConflict->ours_id, $mergeConflict->theirs_id]) + ->with('user') + ->get() + ->keyBy('id'); + + $ours = $sides->get($mergeConflict->ours_id); + $theirs = $sides->get($mergeConflict->theirs_id); + + if (! $ours instanceof Commit || ! $theirs instanceof Commit) { + return null; + } + + $mergeConflict->setRelation('ours', $ours)->setRelation('theirs', $theirs); + + $side = $viewer instanceof User ? $mergeConflict->sideOf($viewer) : null; + $isAuthor = $viewer instanceof User && in_array($viewer->id, [$ours->user_id, $theirs->user_id], true); + $counts = $side instanceof MergeConflictSide || $isAuthor || $mergeConflict->isClosed() + ? $mergeConflict->votes()->toBase()->selectRaw('side, count(*) as votes')->groupBy('side')->pluck('votes', 'side') + : null; + + return [ + 'id' => $mergeConflict->id, + 'closes_at' => $mergeConflict->closes_at->toIso8601String(), + 'is_closed' => $mergeConflict->isClosed(), + 'side' => $side?->value, + 'can_vote' => $viewer instanceof User && ! $side instanceof MergeConflictSide && $mergeConflict->canBeVotedOnBy($viewer), + 'ours' => $this->side($ours, $counts?->get(MergeConflictSide::Ours->value, 0)), + 'theirs' => $this->side($theirs, $counts?->get(MergeConflictSide::Theirs->value, 0)), + ]; + } + + /** + * @return array{id: string, body: string, user: User|null, votes: int|null} + */ + private function side(Commit $reply, mixed $votes): array + { + return [ + 'id' => $reply->id, + 'body' => Str::limit($reply->body, self::ExcerptLength), + 'user' => $reply->user, + 'votes' => is_numeric($votes) ? (int) $votes : null, + ]; + } +} diff --git a/database/factories/MergeConflictFactory.php b/database/factories/MergeConflictFactory.php new file mode 100644 index 000000000..ecb0f50de --- /dev/null +++ b/database/factories/MergeConflictFactory.php @@ -0,0 +1,33 @@ + + */ +final class MergeConflictFactory extends Factory +{ + /** + * @return array + */ + public function definition(): array + { + return [ + 'commit_id' => Commit::factory(), + 'ours_id' => fn (array $attributes): string => Commit::factory()->create(['parent_id' => $attributes['commit_id']])->id, + 'theirs_id' => fn (array $attributes): string => Commit::factory()->create(['parent_id' => $attributes['commit_id']])->id, + 'closes_at' => now()->addHours(MergeConflict::VotingHours), + ]; + } + + public function closed(): self + { + return $this->state(['closes_at' => now()->subMinute()]); + } +} diff --git a/database/factories/MergeConflictVoteFactory.php b/database/factories/MergeConflictVoteFactory.php new file mode 100644 index 000000000..8b4578ae8 --- /dev/null +++ b/database/factories/MergeConflictVoteFactory.php @@ -0,0 +1,29 @@ + + */ +final class MergeConflictVoteFactory extends Factory +{ + /** + * @return array + */ + public function definition(): array + { + return [ + 'merge_conflict_id' => MergeConflict::factory(), + 'user_id' => User::factory(), + 'side' => MergeConflictSide::Ours, + ]; + } +} diff --git a/database/migrations/2026_10_29_090000_create_merge_conflicts_table.php b/database/migrations/2026_10_29_090000_create_merge_conflicts_table.php new file mode 100644 index 000000000..ee5212aa6 --- /dev/null +++ b/database/migrations/2026_10_29_090000_create_merge_conflicts_table.php @@ -0,0 +1,33 @@ +uuid('id')->primary(); + $table->foreignUuid('commit_id')->unique()->constrained()->cascadeOnDelete(); + $table->foreignUuid('ours_id')->constrained('commits')->cascadeOnDelete(); + $table->foreignUuid('theirs_id')->constrained('commits')->cascadeOnDelete(); + $table->timestamp('closes_at'); + $table->timestamps(); + }); + + Schema::create('merge_conflict_votes', function (Blueprint $table): void { + $table->id(); + $table->foreignUuid('merge_conflict_id')->constrained()->cascadeOnDelete(); + $table->foreignUuid('user_id')->constrained()->cascadeOnDelete(); + $table->string('side'); + $table->timestamps(); + + $table->unique(['merge_conflict_id', 'user_id']); + $table->index(['user_id', 'id']); + }); + } +}; diff --git a/resources/js/components/commit-card.tsx b/resources/js/components/commit-card.tsx index c6f4eda85..123cf1c0e 100644 --- a/resources/js/components/commit-card.tsx +++ b/resources/js/components/commit-card.tsx @@ -36,6 +36,7 @@ import { import { HiddenLinkPreviewContext } from '@/components/hidden-link-preview-context'; import { LinkPreview } from '@/components/link-preview'; import { LiveStreamTrigger } from '@/components/live-stream-card'; +import { MergeConflict } from '@/components/merge-conflict'; import { QuotedCommit } from '@/components/quoted-commit'; import { ThreadPositionBadge, @@ -63,7 +64,12 @@ import { shortHash, } from '@/lib/utils'; import { show } from '@/routes/user'; -import type { Commit, CursorPaginated, ThreadCommit } from '@/types'; +import type { + Commit, + CursorPaginated, + MergeConflict as MergeConflictData, + ThreadCommit, +} from '@/types'; export function CommitCard({ commit, @@ -88,6 +94,7 @@ export function CommitCard({ exceptReplyId, threadPosition, graphAfter, + mergeConflict, }: { commit: Commit; previousLane?: string; @@ -112,6 +119,8 @@ export function CommitCard({ hasRailTail?: boolean; exceptReplyId?: string; threadPosition?: ThreadPosition; + /** Two of the commit's replies that disagree, shown under it on its page. */ + mergeConflict?: MergeConflictData | null; }) { const { props: { auth }, @@ -474,6 +483,13 @@ export function CommitCard({ poll={commit.poll} /> )} + {mergeConflict && ( + + )} {commit.quoted_commit_id !== null && ( theirs ? 'ours' : 'theirs'; +} + +function ConflictSide({ + marker, + side, + isPicked, + isWinner, + total, +}: { + marker: string; + side: MergeConflictSide; + isPicked: boolean; + isWinner: boolean; + total: number | null; +}) { + const share = + total === null || total === 0 || side.votes === null + ? null + : Math.round((side.votes / total) * 100); + + return ( +
+

+ {marker} + {side.user !== null && ( + + + {handleOf(side)} + + )} + {isPicked && ( + + + your pick + + )} + {share !== null && ( + + {share}% · {formatExactCount(side.votes ?? 0)} + + )} +

+

+ {side.body} +

+
+ ); +} + +/** + * Two replies Tom found taking opposite sides, shown like the markers of a git merge conflict for readers to resolve. + */ +export function MergeConflict({ + commit, + mergeConflict: initialMergeConflict, +}: { + commit: Pick; + mergeConflict: MergeConflictData; +}) { + const { auth } = usePage().props; + const commitNotFound = useCommitNotFound(); + const now = useNow(); + const [mergeConflict, setMergeConflict] = useState(initialMergeConflict); + const request = useHttp< + { side: Side | '' }, + { mergeConflict: MergeConflictData | null } + >({ side: '' }); + + const isClosed = + mergeConflict.is_closed || new Date(mergeConflict.closes_at) <= now; + const hasCounts = mergeConflict.ours.votes !== null; + const total = hasCounts + ? (mergeConflict.ours.votes ?? 0) + (mergeConflict.theirs.votes ?? 0) + : null; + const winner = isClosed && hasCounts ? winnerOf(mergeConflict) : null; + const canVote = auth.user !== null && mergeConflict.can_vote && !isClosed; + + const status = !isClosed + ? timeLeft(mergeConflict.closes_at, now) + : winner === null + ? hasCounts && total !== 0 + ? 'Resolved: a tie' + : 'Closed' + : `Resolved: kept ${handleOf(mergeConflict[winner])}`; + + const pick = async (side: Side) => { + if (request.processing) { + return; + } + + let isNotFound = false; + request.transform(() => ({ side })); + + try { + const response = await request.submit( + MergeConflictVoteController.store(commit), + { + onHttpException: (response) => { + isNotFound = commitNotFound(commit.id, response); + }, + onError: (errors) => + toast.error( + Object.values(errors)[0] ?? failedVoteMessage, + ), + }, + ); + + if (response?.mergeConflict) { + setMergeConflict(response.mergeConflict); + router.flushAll(); + } + } catch { + if (!isNotFound) { + toast.error(failedVoteMessage); + } + } + }; + + return ( +
+
+ + CONFLICT + + in the replies + + + {status} + +
+ + +

+ ======= +

+ + + {canVote && ( +
+ {(['ours', 'theirs'] as const).map((side) => ( + + ))} +
+ )} +
+ ); +} diff --git a/resources/js/pages/commit/show.tsx b/resources/js/pages/commit/show.tsx index 361dd4e63..cfe229ab1 100644 --- a/resources/js/pages/commit/show.tsx +++ b/resources/js/pages/commit/show.tsx @@ -12,6 +12,7 @@ import { home } from '@/routes'; import type { Commit, CursorPaginated, + MergeConflict, ThreadCommit, ThreadContinuation, } from '@/types'; @@ -23,11 +24,13 @@ export default function Show({ ancestors, continuation, replies, + mergeConflict, }: { commit: Commit; ancestors: ThreadCommit[]; continuation: ThreadContinuation; replies: CursorPaginated | null; + mergeConflict: MergeConflict | null; }) { const { auth } = usePage().props; const { shown, nextId, size, isCapped, isLoading, loadMore } = @@ -117,6 +120,7 @@ export default function Show({ exceptReplyId={shown[0]?.id} endsWithDot={shown.length === 0} threadPosition={positionAt(focusedPosition)} + mergeConflict={mergeConflict} isFocused /> diff --git a/resources/js/pages/operations/features.tsx b/resources/js/pages/operations/features.tsx index fdf9f6398..05cc7d38c 100644 --- a/resources/js/pages/operations/features.tsx +++ b/resources/js/pages/operations/features.tsx @@ -23,6 +23,11 @@ const FEATURES: Record< description: "Members posting and replying as their organization, from the composer's Post as menu and the API. Off until the rest of organization commits ships; commits already posted as an organization stay up either way.", }, + merge_conflicts: { + label: 'merge conflicts', + description: + 'Tom spotting two replies that disagree and readers picking a side. Off stops new threads from being read and hides the conflicts already opened.', + }, }; /** diff --git a/resources/js/types/commit.ts b/resources/js/types/commit.ts index 73c8df690..39449e825 100644 --- a/resources/js/types/commit.ts +++ b/resources/js/types/commit.ts @@ -40,6 +40,23 @@ export type Poll = { options: PollOption[]; }; +export type MergeConflictSide = { + id: string; + body: string; + user: User | null; + votes: number | null; +}; + +export type MergeConflict = { + id: string; + closes_at: string; + is_closed: boolean; + side: 'ours' | 'theirs' | null; + can_vote: boolean; + ours: MergeConflictSide; + theirs: MergeConflictSide; +}; + export type CommitImage = { id: string; url: string; diff --git a/resources/js/types/operations.ts b/resources/js/types/operations.ts index 0751ad89b..d5630ddd3 100644 --- a/resources/js/types/operations.ts +++ b/resources/js/types/operations.ts @@ -36,7 +36,7 @@ export type Connectivity = { }; export type Feature = { - name: 'translations' | 'organization_commits'; + name: 'translations' | 'organization_commits' | 'merge_conflicts'; enabled: boolean; }; diff --git a/routes/web.php b/routes/web.php index 42cd4ddae..0019f5b4c 100644 --- a/routes/web.php +++ b/routes/web.php @@ -67,6 +67,7 @@ use App\Http\Controllers\LogController; use App\Http\Controllers\MentionController; use App\Http\Controllers\MentionSuggestionController; +use App\Http\Controllers\MergeConflictVoteController; use App\Http\Controllers\MessageImageController; use App\Http\Controllers\MessageLinkPreviewController; use App\Http\Controllers\MessagePinController; @@ -462,6 +463,8 @@ ->name('commit-reply.destroy'); Route::post('commits/{commit}/poll/votes', [CommitPollVoteController::class, 'store']) ->name('commit-poll-vote.store'); + Route::post('commits/{commit}/merge-conflict/votes', [MergeConflictVoteController::class, 'store']) + ->name('merge-conflict-vote.store'); // Notifications... Route::get('notifications', [NotificationController::class, 'index'])->name('notification.index'); diff --git a/tests/Feature/Controllers/FeatureControllerTest.php b/tests/Feature/Controllers/FeatureControllerTest.php index 53fc43a38..f1999711a 100644 --- a/tests/Feature/Controllers/FeatureControllerTest.php +++ b/tests/Feature/Controllers/FeatureControllerTest.php @@ -16,7 +16,7 @@ ->assertOk() ->assertInertia(fn ($page) => $page ->component('operations/features') - ->where('features', [['name' => 'translations', 'enabled' => true], ['name' => 'organization_commits', 'enabled' => false]])); + ->where('features', [['name' => 'translations', 'enabled' => true], ['name' => 'organization_commits', 'enabled' => false, ['name' => 'merge_conflicts', 'enabled' => true]])); }); it('hides the features from non admins', function (): void { @@ -33,7 +33,7 @@ $admin = User::factory()->create(['handle' => 'admin']); $this->actingAs($admin)->get(route('feature.index')) - ->assertInertia(fn ($page) => $page->where('features', [['name' => 'translations', 'enabled' => true], ['name' => 'organization_commits', 'enabled' => false]])); + ->assertInertia(fn ($page) => $page->where('features', [['name' => 'translations', 'enabled' => true], ['name' => 'organization_commits', 'enabled' => false, ['name' => 'merge_conflicts', 'enabled' => true]])); $this->fromRoute('feature.index') ->put(route('feature.update', 'translations'), ['enabled' => false]) @@ -42,7 +42,7 @@ expect(resolve(Features::class)->enabled(Feature::Translations))->toBeFalse(); $this->assertDatabaseHas('features', ['name' => 'translations', 'enabled' => false]); $this->get(route('feature.index')) - ->assertInertia(fn ($page) => $page->where('features', [['name' => 'translations', 'enabled' => false], ['name' => 'organization_commits', 'enabled' => false]])); + ->assertInertia(fn ($page) => $page->where('features', [['name' => 'translations', 'enabled' => false], ['name' => 'organization_commits', 'enabled' => false, ['name' => 'merge_conflicts', 'enabled' => true]])); $this->put(route('feature.update', 'translations'), ['enabled' => true])->assertRedirect(); diff --git a/tests/Feature/Controllers/MergeConflictVoteControllerTest.php b/tests/Feature/Controllers/MergeConflictVoteControllerTest.php new file mode 100644 index 000000000..a76725992 --- /dev/null +++ b/tests/Feature/Controllers/MergeConflictVoteControllerTest.php @@ -0,0 +1,116 @@ +create(); + MergeConflictVote::factory()->for($mergeConflict)->create(['side' => 'theirs']); + + $this->actingAs($voter = User::factory()->create()) + ->postJson(route('merge-conflict-vote.store', $mergeConflict->commit), ['side' => 'ours']) + ->assertOk() + ->assertJsonPath('mergeConflict.side', 'ours') + ->assertJsonPath('mergeConflict.can_vote', false) + ->assertJsonPath('mergeConflict.ours.votes', 1) + ->assertJsonPath('mergeConflict.theirs.votes', 1); + + $this->assertDatabaseHas('merge_conflict_votes', ['merge_conflict_id' => $mergeConflict->id, 'user_id' => $voter->id, 'side' => 'ours']); +}); + +it('keeps the first side a reader picked', function (): void { + $mergeConflict = MergeConflict::factory()->create(); + $voter = User::factory()->create(); + MergeConflictVote::factory()->for($mergeConflict)->for($voter)->create(['side' => 'ours']); + + $this->actingAs($voter) + ->postJson(route('merge-conflict-vote.store', $mergeConflict->commit), ['side' => 'theirs']) + ->assertOk() + ->assertJsonPath('mergeConflict.side', 'ours'); + + expect($mergeConflict->votes()->count())->toBe(1); +}); + +it('does not let the authors of either side vote', function (string $side): void { + $mergeConflict = MergeConflict::factory()->create(); + + $this->actingAs($mergeConflict->{$side}->user) + ->postJson(route('merge-conflict-vote.store', $mergeConflict->commit), ['side' => 'ours']) + ->assertForbidden(); +})->with(['ours', 'theirs']); + +it('does not take votes once the conflict closed', function (): void { + $mergeConflict = MergeConflict::factory()->closed()->create(); + + $this->actingAs(User::factory()->create()) + ->postJson(route('merge-conflict-vote.store', $mergeConflict->commit), ['side' => 'ours']) + ->assertForbidden(); +}); + +it('only takes ours or theirs', function (): void { + $mergeConflict = MergeConflict::factory()->create(); + + $this->actingAs(User::factory()->create()) + ->postJson(route('merge-conflict-vote.store', $mergeConflict->commit), ['side' => 'both']) + ->assertJsonValidationErrors('side'); +}); + +it('requires authentication to vote', function (): void { + $mergeConflict = MergeConflict::factory()->create(); + + $this->postJson(route('merge-conflict-vote.store', $mergeConflict->commit), ['side' => 'ours']) + ->assertUnauthorized(); +}); + +it('shows the merge conflict on the commit, hiding the count until the reader picks a side', function (): void { + $mergeConflict = MergeConflict::factory()->create(); + MergeConflictVote::factory()->for($mergeConflict)->count(2)->create(); + + $this->actingAs(User::factory()->create()) + ->get(route('commit.show', ['user' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) + ->assertOk() + ->assertInertia(fn ($page) => $page + ->where('mergeConflict.ours.id', $mergeConflict->ours_id) + ->where('mergeConflict.ours.body', $mergeConflict->ours->body) + ->where('mergeConflict.ours.user.handle', $mergeConflict->ours->user->handle) + ->where('mergeConflict.theirs.id', $mergeConflict->theirs_id) + ->where('mergeConflict.ours.votes', null) + ->where('mergeConflict.side', null) + ->where('mergeConflict.can_vote', true) + ->where('mergeConflict.is_closed', false)); +}); + +it('shows everyone the final count once the conflict closed', function (): void { + $mergeConflict = MergeConflict::factory()->closed()->create(); + MergeConflictVote::factory()->for($mergeConflict)->count(2)->create(['side' => 'theirs']); + + $this->get(route('commit.show', ['user' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) + ->assertOk() + ->assertInertia(fn ($page) => $page + ->where('mergeConflict.is_closed', true) + ->where('mergeConflict.ours.votes', 0) + ->where('mergeConflict.theirs.votes', 2)); +}); + +it('hides the merge conflict when the reader cannot see one side', function (): void { + $mergeConflict = MergeConflict::factory()->create(); + $reader = User::factory()->create(); + $reader->blocking()->attach($mergeConflict->theirs->user); + + $this->actingAs($reader) + ->get(route('commit.show', ['user' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) + ->assertInertia(fn ($page) => $page->where('mergeConflict', null)); +}); + +it('hides merge conflicts while they are switched off', function (): void { + $mergeConflict = MergeConflict::factory()->create(); + resolve(Features::class)->update(Feature::MergeConflicts, false); + + $this->get(route('commit.show', ['user' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) + ->assertInertia(fn ($page) => $page->where('mergeConflict', null)); +}); diff --git a/tests/Unit/Actions/CreateMergeConflictTest.php b/tests/Unit/Actions/CreateMergeConflictTest.php new file mode 100644 index 000000000..d4f4b4819 --- /dev/null +++ b/tests/Unit/Actions/CreateMergeConflictTest.php @@ -0,0 +1,129 @@ + + */ +function repliesTo(Commit $commit, string ...$bodies): array +{ + return array_values(array_map(fn (string $body, int $index): Commit => Commit::factory()->create([ + 'parent_id' => $commit->id, + 'body' => $body, + 'created_at' => now()->subMinutes(count($bodies) - $index), + ]), $bodies, array_keys($bodies))); +} + +it('opens a merge conflict between two replies that disagree', function (): void { + ConflictSpotter::fake([['conflict' => true, 'ours' => 1, 'theirs' => 3]])->preventStrayPrompts(); + $commit = Commit::factory()->create(['body' => 'Tabs or spaces?']); + [$tabs, , $spaces] = repliesTo($commit, 'Tabs, always.', 'Nice question', 'Spaces, tabs are wrong.'); + + $mergeConflict = resolve(CreateMergeConflict::class)->handle($commit); + + expect($mergeConflict) + ->commit_id->toBe($commit->id) + ->ours_id->toBe($tabs->id) + ->theirs_id->toBe($spaces->id) + ->closes_at->toDateTimeString()->toBe(now()->addHours(24)->toDateTimeString()); + ConflictSpotter::assertPrompted(fn (AgentPrompt $prompt): bool => str_contains($prompt->prompt, '"post":"Tabs or spaces?"') + && str_contains($prompt->prompt, '"number":3') + && str_contains($prompt->prompt, '"body":"Spaces, tabs are wrong."')); +}); + +it('puts the earlier reply on our side whichever way Tom names them', function (): void { + ConflictSpotter::fake([['conflict' => true, 'ours' => 2, 'theirs' => 1]]); + $commit = Commit::factory()->create(); + [$first, $second] = repliesTo($commit, 'Yes', 'No'); + + expect(resolve(CreateMergeConflict::class)->handle($commit)) + ->ours_id->toBe($first->id) + ->theirs_id->toBe($second->id); +}); + +it('opens nothing when Tom finds no conflict, or names replies that do not make one', function (array $answer): void { + ConflictSpotter::fake([$answer]); + $commit = Commit::factory()->create(); + $author = User::factory()->create(); + repliesTo($commit, 'Yes', 'No'); + Commit::factory()->for($author)->create(['parent_id' => $commit->id, 'created_at' => now()]); + Commit::factory()->for($author)->create(['parent_id' => $commit->id, 'created_at' => now()->addSecond()]); + + expect(resolve(CreateMergeConflict::class)->handle($commit))->toBeNull(); + $this->assertDatabaseCount('merge_conflicts', 0); +})->with([ + 'no conflict' => [['conflict' => false, 'ours' => 0, 'theirs' => 0]], + 'out of range' => [['conflict' => true, 'ours' => 1, 'theirs' => 9]], + 'the same reply twice' => [['conflict' => true, 'ours' => 1, 'theirs' => 1]], + 'one author against themselves' => [['conflict' => true, 'ours' => 3, 'theirs' => 4]], + 'a malformed answer' => [['conflict' => 'yes']], +]); + +it('does not ask Tom until two people replied', function (): void { + ConflictSpotter::fake()->preventStrayPrompts(); + $commit = Commit::factory()->create(); + $author = User::factory()->create(); + Commit::factory()->for($author)->count(2)->create(['parent_id' => $commit->id]); + Commit::factory()->pending()->create(['parent_id' => $commit->id]); + + expect(resolve(CreateMergeConflict::class)->handle($commit))->toBeNull(); + ConflictSpotter::assertNeverPrompted(); +}); + +it('leaves the replies of Tom out', function (): void { + ConflictSpotter::fake()->preventStrayPrompts(); + $tom = User::factory()->create(['github_id' => CreateTomFriendship::GITHUB_ID, 'handle' => CreateTomFriendship::HANDLE]); + $commit = Commit::factory()->create(); + repliesTo($commit, 'Yes'); + Commit::factory()->for($tom)->create(['parent_id' => $commit->id, 'body' => 'No']); + + expect(resolve(CreateMergeConflict::class)->handle($commit))->toBeNull(); + ConflictSpotter::assertNeverPrompted(); +}); + +it('reads a thread at most once an hour', function (): void { + ConflictSpotter::fake([ + ['conflict' => false, 'ours' => 0, 'theirs' => 0], + ['conflict' => true, 'ours' => 1, 'theirs' => 2], + ]); + $commit = Commit::factory()->create(); + repliesTo($commit, 'Yes', 'No'); + + resolve(CreateMergeConflict::class)->handle($commit); + resolve(CreateMergeConflict::class)->handle($commit); + ConflictSpotter::assertPromptedTimes(1); + + $this->travel(61)->minutes(); + + expect(resolve(CreateMergeConflict::class)->handle($commit))->toBeInstanceOf(MergeConflict::class); + ConflictSpotter::assertPromptedTimes(2); +}); + +it('opens one merge conflict per commit', function (): void { + ConflictSpotter::fake()->preventStrayPrompts(); + $mergeConflict = MergeConflict::factory()->create(); + repliesTo($mergeConflict->commit, 'Yes', 'No'); + + expect(resolve(CreateMergeConflict::class)->handle($mergeConflict->commit))->toBeNull(); + ConflictSpotter::assertNeverPrompted(); +}); + +it('reads no thread while merge conflicts are switched off', function (): void { + ConflictSpotter::fake()->preventStrayPrompts(); + resolve(Features::class)->update(Feature::MergeConflicts, false); + $commit = Commit::factory()->create(); + repliesTo($commit, 'Yes', 'No'); + + expect(resolve(CreateMergeConflict::class)->handle($commit))->toBeNull(); + ConflictSpotter::assertNeverPrompted(); +}); diff --git a/tests/Unit/Actions/UpdateCommitStatusesTest.php b/tests/Unit/Actions/UpdateCommitStatusesTest.php index a8a2b361f..79af386a2 100644 --- a/tests/Unit/Actions/UpdateCommitStatusesTest.php +++ b/tests/Unit/Actions/UpdateCommitStatusesTest.php @@ -8,6 +8,7 @@ use App\Enums\CommitStatus; use App\Jobs\CheckUserLevel; use App\Jobs\ReplyAsTom; +use App\Jobs\SpotMergeConflict; use App\Models\Commit; use App\Models\Headline; use App\Models\User; @@ -51,7 +52,7 @@ $queryCounts = collect([2, 6])->map(function (int $count) use ($tom): int { $parents = Commit::factory()->for($tom)->count($count)->create(['body' => 'A developer community.']); $replies = $parents->map(fn (Commit $parent): Commit => Commit::factory()->pending()->for($parent, 'parent')->create(['body' => 'Thanks for explaining!'])); - Queue::fake([ReplyAsTom::class, CheckUserLevel::class]); + Queue::fake([ReplyAsTom::class, CheckUserLevel::class, SpotMergeConflict::class]); DB::flushQueryLog(); DB::enableQueryLog(); diff --git a/tests/Unit/Jobs/SpotMergeConflictTest.php b/tests/Unit/Jobs/SpotMergeConflictTest.php new file mode 100644 index 000000000..65688b868 --- /dev/null +++ b/tests/Unit/Jobs/SpotMergeConflictTest.php @@ -0,0 +1,40 @@ +create(); + + SpotMergeConflict::dispatchFor(...Commit::factory()->count(2)->create(['parent_id' => $commit->id])->all()); + + Queue::assertPushedTimes(SpotMergeConflict::class, 1); + Queue::assertPushed(SpotMergeConflict::class, fn (SpotMergeConflict $job): bool => $job->commitId === $commit->id + && $job->delay?->toDateTimeString() === now()->addMinutes(5)->toDateTimeString()); +}); + +it('looks at the thread once a pending reply is approved', function (): void { + Queue::fake([SpotMergeConflict::class]); + $commit = Commit::factory()->create(); + $replies = Commit::factory()->pending()->count(2)->create(['parent_id' => $commit->id]); + + resolve(UpdateCommitStatuses::class)->handle($replies->modelKeys(), CommitStatus::Approved); + + Queue::assertPushedTimes(SpotMergeConflict::class, 1); +}); + +it('leaves commits that are not replies alone', function (): void { + Queue::fake([SpotMergeConflict::class]); + + resolve(CreateCommit::class)->handle(User::factory()->create(), 'Hello'); + + Queue::assertNotPushed(SpotMergeConflict::class); +}); diff --git a/tests/Unit/MergeConflict.test.tsx b/tests/Unit/MergeConflict.test.tsx new file mode 100644 index 000000000..a03a4cf4c --- /dev/null +++ b/tests/Unit/MergeConflict.test.tsx @@ -0,0 +1,126 @@ +/// +import assert from 'node:assert/strict'; +import { App } from '@inertiajs/react'; +import type { ComponentProps } from 'react'; +import { renderToStaticMarkup } from 'react-dom/server'; +import { test } from 'vite-plus/test'; +import { MergeConflict } from '../../resources/js/components/merge-conflict'; +import { TooltipProvider } from '../../resources/js/components/ui/tooltip'; +import type { + MergeConflict as MergeConflictData, + User, +} from '../../resources/js/types'; + +const commit = { id: '019a2b3c-0000-7000-8000-000000000001' }; + +function author(handle: string): User { + return { + id: handle, + handle, + name: handle, + avatar_url: null, + lane: 0, + } as User; +} + +function conflict( + overrides: Partial = {}, +): MergeConflictData { + return { + id: 'conflict', + closes_at: '2999-01-01T00:00:00Z', + is_closed: false, + side: null, + can_vote: true, + ours: { + id: 'ours', + body: 'Tabs, always.', + user: author('alice'), + votes: null, + }, + theirs: { + id: 'theirs', + body: 'Spaces. Tabs are wrong.', + user: author('bob'), + votes: null, + }, + ...overrides, + }; +} + +function render(mergeConflict: MergeConflictData, signedIn = true): string { + return renderToStaticMarkup( + ( + + + + )} + initialPage={ + { + component: 'Commit', + url: '/', + version: null, + props: { + errors: {}, + auth: { user: signedIn ? { id: 'reader' } : null }, + }, + clearHistory: false, + encryptHistory: false, + } as unknown as ComponentProps['initialPage'] + } + />, + ).replaceAll('', ''); +} + +test('shows both replies between conflict markers, with a button to accept each', () => { + const html = render(conflict()); + + assert.match( + html, + /<<<<<<<.*@alice.*Tabs, always\..*=======.*>>>>>>>.*@bob.*Spaces\. Tabs are wrong\./s, + ); + assert.match(html, /data-test="merge-conflict-ours"[^>]*>.*Accept @alice/s); + assert.match(html, /data-test="merge-conflict-theirs"[^>]*>.*Accept @bob/s); + assert.doesNotMatch(html, /%/); +}); + +test('shows the reader their pick and the count once they picked a side', () => { + const html = render( + conflict({ + side: 'theirs', + can_vote: false, + ours: { ...conflict().ours, votes: 1 }, + theirs: { ...conflict().theirs, votes: 3 }, + }), + ); + + assert.match(html, /25% · 1/); + assert.match(html, /@bob.*your pick.*75% · 3/s); + assert.doesNotMatch(html, /Accept @/); +}); + +test('names the side that won once the conflict closed, or a tie', () => { + const closed = { + is_closed: true, + can_vote: false, + ours: { ...conflict().ours, votes: 5 }, + theirs: { ...conflict().theirs, votes: 2 }, + }; + + assert.match(render(conflict(closed)), /Resolved: kept @alice/); + assert.match( + render(conflict({ ...closed, theirs: { ...closed.theirs, votes: 5 } })), + /Resolved: a tie/, + ); +}); + +test('offers guests no buttons', () => { + assert.doesNotMatch( + render(conflict({ can_vote: false }), false), + /Accept @/, + ); +}); From 19ed9dddf5d0dd0109bc8580d7b65b420962b944 Mon Sep 17 00:00:00 2001 From: Houssem Eddine BENABDALLAH Date: Thu, 29 Oct 2026 16:44:09 +0100 Subject: [PATCH 2/7] fix: let a failed read retry and only take votes from readers who see both sides --- app/Actions/CreateMergeConflict.php | 24 ++++++++++++------- .../CreateMergeConflictVoteRequest.php | 5 ++++ app/Models/MergeConflict.php | 9 ++++--- .../MergeConflictVoteControllerTest.php | 17 +++++++++++++ .../Unit/Actions/CreateMergeConflictTest.php | 16 +++++++++++++ 5 files changed, 60 insertions(+), 11 deletions(-) diff --git a/app/Actions/CreateMergeConflict.php b/app/Actions/CreateMergeConflict.php index 8b9d921f5..ff45220f4 100644 --- a/app/Actions/CreateMergeConflict.php +++ b/app/Actions/CreateMergeConflict.php @@ -14,6 +14,7 @@ use App\Services\Features; use Illuminate\Database\Eloquent\Collection; use Illuminate\Support\Facades\Cache; +use Throwable; /** * Asks Tom whether two replies to a commit take opposite sides, and opens a merge conflict between them when they do. @@ -53,14 +54,21 @@ public function handle(Commit $commit): ?MergeConflict return null; } - $response = new ConflictSpotter()->prompt(collect([ - 'post' => mb_substr($commit->body, 0, self::MaxReplyLength), - 'replies' => $replies->values()->map(fn (Commit $reply, int $index): array => [ - 'number' => $index + 1, - 'author' => '@'.$reply->user?->handle, - 'body' => mb_substr($reply->body, 0, self::MaxReplyLength), - ]), - ])->toJson()); + try { + $response = new ConflictSpotter()->prompt(collect([ + 'post' => mb_substr($commit->body, 0, self::MaxReplyLength), + 'replies' => $replies->values()->map(fn (Commit $reply, int $index): array => [ + 'number' => $index + 1, + 'author' => '@'.$reply->user?->handle, + 'body' => mb_substr($reply->body, 0, self::MaxReplyLength), + ]), + ])->toJson()); + } catch (Throwable $throwable) { + // A read that failed doesn't count, so the job can retry it. + Cache::forget("merge-conflict-read:{$commit->id}"); + + throw $throwable; + } $ours = $replies->values()->get(is_int($response['ours'] ?? null) ? $response['ours'] - 1 : -1); $theirs = $replies->values()->get(is_int($response['theirs'] ?? null) ? $response['theirs'] - 1 : -1); diff --git a/app/Http/Requests/CreateMergeConflictVoteRequest.php b/app/Http/Requests/CreateMergeConflictVoteRequest.php index b0c645764..bd82aeeb8 100644 --- a/app/Http/Requests/CreateMergeConflictVoteRequest.php +++ b/app/Http/Requests/CreateMergeConflictVoteRequest.php @@ -8,6 +8,7 @@ use App\Models\Commit; use App\Models\MergeConflict; use App\Models\User; +use App\Queries\MergeConflictQuery; use Illuminate\Contracts\Validation\ValidationRule; use Illuminate\Foundation\Http\FormRequest; use Illuminate\Validation\Rule; @@ -15,12 +16,16 @@ final class CreateMergeConflictVoteRequest extends FormRequest { + /** + * Only readers who are shown the merge conflict, so who can see both sides, get to vote on it. + */ public function authorize(): bool { $mergeConflict = $this->commit()->mergeConflict; return $this->commit()->isVisibleTo($this->voter()) && $mergeConflict instanceof MergeConflict + && resolve(MergeConflictQuery::class)->get($this->commit(), $this->voter()) !== null && $mergeConflict->canBeVotedOnBy($this->voter()); } diff --git a/app/Models/MergeConflict.php b/app/Models/MergeConflict.php index f6464b379..00db47dd9 100644 --- a/app/Models/MergeConflict.php +++ b/app/Models/MergeConflict.php @@ -25,8 +25,8 @@ * @property-read CarbonInterface $created_at * @property-read CarbonInterface $updated_at * @property-read Commit $commit - * @property-read Commit $ours - * @property-read Commit $theirs + * @property-read Commit|null $ours + * @property-read Commit|null $theirs * @property-read Collection $votes */ final class MergeConflict extends Model @@ -99,7 +99,10 @@ public function isClosed(): bool */ public function canBeVotedOnBy(User $user): bool { - return ! $this->isClosed() && ! in_array($user->id, [$this->ours->user_id, $this->theirs->user_id], true); + return ! $this->isClosed() + && $this->ours instanceof Commit + && $this->theirs instanceof Commit + && ! in_array($user->id, [$this->ours->user_id, $this->theirs->user_id], true); } public function sideOf(User $user): ?MergeConflictSide diff --git a/tests/Feature/Controllers/MergeConflictVoteControllerTest.php b/tests/Feature/Controllers/MergeConflictVoteControllerTest.php index a76725992..23e1258da 100644 --- a/tests/Feature/Controllers/MergeConflictVoteControllerTest.php +++ b/tests/Feature/Controllers/MergeConflictVoteControllerTest.php @@ -52,6 +52,23 @@ ->assertForbidden(); }); +it('does not let a reader vote on a conflict they are not shown', function (Closure $hide): void { + $mergeConflict = MergeConflict::factory()->create(); + $voter = User::factory()->create(); + $hide($mergeConflict, $voter); + + $this->actingAs($voter) + ->postJson(route('merge-conflict-vote.store', $mergeConflict->commit), ['side' => 'ours']) + ->assertForbidden(); + + $this->assertDatabaseCount('merge_conflict_votes', 0); +})->with([ + 'switched off' => [fn () => resolve(Features::class)->update(Feature::MergeConflicts, false)], + 'a side is blocked' => [fn (MergeConflict $mergeConflict, User $voter) => $voter->blocking()->attach($mergeConflict->theirs->user)], + 'a side is muted' => [fn (MergeConflict $mergeConflict, User $voter) => $voter->muting()->attach($mergeConflict->ours->user)], + 'a side is deleted' => [fn (MergeConflict $mergeConflict) => $mergeConflict->theirs->delete()], +]); + it('only takes ours or theirs', function (): void { $mergeConflict = MergeConflict::factory()->create(); diff --git a/tests/Unit/Actions/CreateMergeConflictTest.php b/tests/Unit/Actions/CreateMergeConflictTest.php index d4f4b4819..3c1ca5938 100644 --- a/tests/Unit/Actions/CreateMergeConflictTest.php +++ b/tests/Unit/Actions/CreateMergeConflictTest.php @@ -109,6 +109,22 @@ function repliesTo(Commit $commit, string ...$bodies): array ConflictSpotter::assertPromptedTimes(2); }); +it('reads the thread again when Tom failed to answer', function (): void { + $prompts = 0; + ConflictSpotter::fake(function () use (&$prompts): array { + throw_if($prompts++ === 0, RuntimeException::class, 'Tom is down'); + + return ['conflict' => true, 'ours' => 1, 'theirs' => 2]; + }); + $commit = Commit::factory()->create(); + repliesTo($commit, 'Yes', 'No'); + + expect(fn (): ?MergeConflict => resolve(CreateMergeConflict::class)->handle($commit))->toThrow(RuntimeException::class); + + expect(resolve(CreateMergeConflict::class)->handle($commit))->toBeInstanceOf(MergeConflict::class) + ->and($prompts)->toBe(2); +}); + it('opens one merge conflict per commit', function (): void { ConflictSpotter::fake()->preventStrayPrompts(); $mergeConflict = MergeConflict::factory()->create(); From 223441b6f54b8553a31362dca200b9dfcfc7b37d Mon Sep 17 00:00:00 2001 From: Houssem Eddine BENABDALLAH Date: Thu, 29 Oct 2026 18:01:53 +0100 Subject: [PATCH 3/7] fix: read a thread again once the hour is up when a reply came in meanwhile --- app/Actions/CreateMergeConflict.php | 14 +++++++++++++- app/Jobs/SpotMergeConflict.php | 7 +++++-- tests/Unit/Jobs/SpotMergeConflictTest.php | 16 ++++++++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/app/Actions/CreateMergeConflict.php b/app/Actions/CreateMergeConflict.php index ff45220f4..c7354e5b7 100644 --- a/app/Actions/CreateMergeConflict.php +++ b/app/Actions/CreateMergeConflict.php @@ -12,8 +12,10 @@ use App\Models\User; use App\Queries\TomQuery; use App\Services\Features; +use Carbon\CarbonInterface; use Illuminate\Database\Eloquent\Collection; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Date; use Throwable; /** @@ -50,7 +52,7 @@ public function handle(Commit $commit): ?MergeConflict $replies = $this->replies($commit); if ($replies->pluck('user_id')->unique()->count() < 2 - || ! Cache::add("merge-conflict-read:{$commit->id}", true, self::ReadEvery)) { + || ! Cache::add("merge-conflict-read:{$commit->id}", now()->addSeconds(self::ReadEvery)->getTimestamp(), self::ReadEvery)) { return null; } @@ -88,6 +90,16 @@ public function handle(Commit $commit): ?MergeConflict ]); } + /** + * When Tom may read the thread again: right away, or once the hour since he last read it is up. + */ + public function nextReadAt(string $commitId): CarbonInterface + { + $readAgainAt = Cache::get("merge-conflict-read:{$commitId}"); + + return is_int($readAgainAt) ? Date::createFromTimestamp($readAgainAt) : now(); + } + /** * @return Collection */ diff --git a/app/Jobs/SpotMergeConflict.php b/app/Jobs/SpotMergeConflict.php index 62290395c..53255bda7 100644 --- a/app/Jobs/SpotMergeConflict.php +++ b/app/Jobs/SpotMergeConflict.php @@ -31,14 +31,17 @@ public function __construct(public readonly string $commitId) } /** - * Looks at the threads the given approved replies are in, once each. + * Looks at the threads the given approved replies are in, once each. A thread Tom read within the hour waits until + * he may read it again, so a reply that came in meanwhile still gets read. */ public static function dispatchFor(Commit ...$replies): void { $parentIds = array_unique(array_filter(array_map(fn (Commit $reply): ?string => $reply->parent_id, $replies))); foreach ($parentIds as $parentId) { - dispatch(new self($parentId))->delay(now()->addMinutes(5))->afterCommit(); + $readAt = resolve(CreateMergeConflict::class)->nextReadAt($parentId)->max(now()->addMinutes(5)); + + dispatch(new self($parentId))->delay($readAt)->afterCommit(); } } diff --git a/tests/Unit/Jobs/SpotMergeConflictTest.php b/tests/Unit/Jobs/SpotMergeConflictTest.php index 65688b868..0ec437071 100644 --- a/tests/Unit/Jobs/SpotMergeConflictTest.php +++ b/tests/Unit/Jobs/SpotMergeConflictTest.php @@ -3,7 +3,9 @@ declare(strict_types=1); use App\Actions\CreateCommit; +use App\Actions\CreateMergeConflict; use App\Actions\UpdateCommitStatuses; +use App\Ai\Agents\ConflictSpotter; use App\Enums\CommitStatus; use App\Jobs\SpotMergeConflict; use App\Models\Commit; @@ -21,6 +23,20 @@ && $job->delay?->toDateTimeString() === now()->addMinutes(5)->toDateTimeString()); }); +it('waits for Tom to be able to read a thread he read within the hour', function (): void { + ConflictSpotter::fake([['conflict' => false, 'ours' => 0, 'theirs' => 0]]); + $commit = Commit::factory()->create(); + Commit::factory()->count(2)->create(['parent_id' => $commit->id]); + resolve(CreateMergeConflict::class)->handle($commit); + $readAgainAt = now()->addHour(); + Queue::fake([SpotMergeConflict::class]); + + $this->travel(10)->minutes(); + SpotMergeConflict::dispatchFor(Commit::factory()->create(['parent_id' => $commit->id])); + + Queue::assertPushed(SpotMergeConflict::class, fn (SpotMergeConflict $job): bool => $job->delay?->toDateTimeString() === $readAgainAt->toDateTimeString()); +}); + it('looks at the thread once a pending reply is approved', function (): void { Queue::fake([SpotMergeConflict::class]); $commit = Commit::factory()->create(); From b0f8e50dd1ac04384ceeff66a5bc94e354421c3b Mon Sep 17 00:00:00 2001 From: Houssem Eddine BENABDALLAH Date: Thu, 29 Oct 2026 18:23:43 +0100 Subject: [PATCH 4/7] fix: fetch the final count when voting closes while the page is open --- resources/js/components/merge-conflict.tsx | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/resources/js/components/merge-conflict.tsx b/resources/js/components/merge-conflict.tsx index 4647c0aec..e3c24770f 100644 --- a/resources/js/components/merge-conflict.tsx +++ b/resources/js/components/merge-conflict.tsx @@ -1,6 +1,6 @@ import { Link, router, useHttp, usePage } from '@inertiajs/react'; import { Check, GitMerge } from 'lucide-react'; -import { useState } from 'react'; +import { useEffect, useState } from 'react'; import { toast } from 'sonner'; import MergeConflictVoteController from '@/actions/App/Http/Controllers/MergeConflictVoteController'; import { timeLeft } from '@/components/commit-poll'; @@ -109,6 +109,12 @@ export function MergeConflict({ const commitNotFound = useCommitNotFound(); const now = useNow(); const [mergeConflict, setMergeConflict] = useState(initialMergeConflict); + const [previousInitial, setPreviousInitial] = + useState(initialMergeConflict); + if (initialMergeConflict !== previousInitial) { + setPreviousInitial(initialMergeConflict); + setMergeConflict(initialMergeConflict); + } const request = useHttp< { side: Side | '' }, { mergeConflict: MergeConflictData | null } @@ -122,6 +128,14 @@ export function MergeConflict({ : null; const winner = isClosed && hasCounts ? winnerOf(mergeConflict) : null; const canVote = auth.user !== null && mergeConflict.can_vote && !isClosed; + const nowTimestamp = now.getTime(); + + // Voting closed while the page was open: fetch the final count, which the reader may not have seen yet. + useEffect(() => { + if (isClosed && !hasCounts) { + router.reload({ only: ['mergeConflict'] }); + } + }, [isClosed, hasCounts, nowTimestamp]); const status = !isClosed ? timeLeft(mergeConflict.closes_at, now) From 59d9cded93eb0663793457f812d74c97b3f3fa5a Mon Sep 17 00:00:00 2001 From: Houssem Eddine BENABDALLAH Date: Thu, 29 Oct 2026 23:36:21 +0100 Subject: [PATCH 5/7] fix: queue a read for replies that come in during one and show the final count once voting closes --- app/Actions/CreateMergeConflict.php | 5 +++-- app/Jobs/SpotMergeConflict.php | 7 ++++--- resources/js/components/merge-conflict.tsx | 12 +++++++----- tests/Unit/Jobs/SpotMergeConflictTest.php | 19 ++++++++++++++++++- tests/Unit/MergeConflict.test.tsx | 14 ++++++++++++++ 5 files changed, 46 insertions(+), 11 deletions(-) diff --git a/app/Actions/CreateMergeConflict.php b/app/Actions/CreateMergeConflict.php index c7354e5b7..2a87c0393 100644 --- a/app/Actions/CreateMergeConflict.php +++ b/app/Actions/CreateMergeConflict.php @@ -91,13 +91,14 @@ public function handle(Commit $commit): ?MergeConflict } /** - * When Tom may read the thread again: right away, or once the hour since he last read it is up. + * When Tom may read the thread again: right away, or once the hour since he last read it is up. A second later than + * the hour, as the cache may let the slot go a little after it. */ public function nextReadAt(string $commitId): CarbonInterface { $readAgainAt = Cache::get("merge-conflict-read:{$commitId}"); - return is_int($readAgainAt) ? Date::createFromTimestamp($readAgainAt) : now(); + return is_int($readAgainAt) ? Date::createFromTimestamp($readAgainAt)->addSecond() : now(); } /** diff --git a/app/Jobs/SpotMergeConflict.php b/app/Jobs/SpotMergeConflict.php index 53255bda7..dd8550931 100644 --- a/app/Jobs/SpotMergeConflict.php +++ b/app/Jobs/SpotMergeConflict.php @@ -6,7 +6,7 @@ use App\Actions\CreateMergeConflict; use App\Models\Commit; -use Illuminate\Contracts\Queue\ShouldBeUnique; +use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Queue\Queueable; use Illuminate\Queue\Attributes\Backoff; @@ -15,13 +15,14 @@ use Illuminate\Queue\Attributes\UniqueFor; /** - * Waits a few minutes after a reply, so a burst of replies is read together. + * Waits a few minutes after a reply, so a burst of replies is read together. The lock lets go once the read starts, so a + * reply that comes in during it queues the next read. */ #[Backoff([60, 600])] #[Timeout(120)] #[Tries(2)] #[UniqueFor(3600)] -final class SpotMergeConflict implements ShouldBeUnique, ShouldQueue +final class SpotMergeConflict implements ShouldBeUniqueUntilProcessing, ShouldQueue { use Queueable; diff --git a/resources/js/components/merge-conflict.tsx b/resources/js/components/merge-conflict.tsx index e3c24770f..55f677156 100644 --- a/resources/js/components/merge-conflict.tsx +++ b/resources/js/components/merge-conflict.tsx @@ -50,9 +50,11 @@ function ConflictSide({ total: number | null; }) { const share = - total === null || total === 0 || side.votes === null + total === null || side.votes === null ? null - : Math.round((side.votes / total) * 100); + : total === 0 + ? 0 + : Math.round((side.votes / total) * 100); return (
@@ -130,12 +132,12 @@ export function MergeConflict({ const canVote = auth.user !== null && mergeConflict.can_vote && !isClosed; const nowTimestamp = now.getTime(); - // Voting closed while the page was open: fetch the final count, which the reader may not have seen yet. + // Voting closed while the page was open: fetch the final count, as the one the reader has may be missing or stale. useEffect(() => { - if (isClosed && !hasCounts) { + if (isClosed && !mergeConflict.is_closed) { router.reload({ only: ['mergeConflict'] }); } - }, [isClosed, hasCounts, nowTimestamp]); + }, [isClosed, mergeConflict.is_closed, nowTimestamp]); const status = !isClosed ? timeLeft(mergeConflict.closes_at, now) diff --git a/tests/Unit/Jobs/SpotMergeConflictTest.php b/tests/Unit/Jobs/SpotMergeConflictTest.php index 0ec437071..c37a4f3de 100644 --- a/tests/Unit/Jobs/SpotMergeConflictTest.php +++ b/tests/Unit/Jobs/SpotMergeConflictTest.php @@ -10,6 +10,8 @@ use App\Jobs\SpotMergeConflict; use App\Models\Commit; use App\Models\User; +use Illuminate\Queue\CallQueuedHandler; +use Illuminate\Queue\Jobs\FakeJob; use Illuminate\Support\Facades\Queue; it('looks at the thread of each reply once, a few minutes later', function (): void { @@ -28,7 +30,7 @@ $commit = Commit::factory()->create(); Commit::factory()->count(2)->create(['parent_id' => $commit->id]); resolve(CreateMergeConflict::class)->handle($commit); - $readAgainAt = now()->addHour(); + $readAgainAt = now()->addHour()->addSecond(); Queue::fake([SpotMergeConflict::class]); $this->travel(10)->minutes(); @@ -37,6 +39,21 @@ Queue::assertPushed(SpotMergeConflict::class, fn (SpotMergeConflict $job): bool => $job->delay?->toDateTimeString() === $readAgainAt->toDateTimeString()); }); +it('queues another look for a reply that comes in while Tom reads the thread', function (): void { + Queue::fake([SpotMergeConflict::class]); + $commit = Commit::factory()->create(); + SpotMergeConflict::dispatchFor(...Commit::factory()->count(2)->create(['parent_id' => $commit->id])->all()); + ConflictSpotter::fake([function () use ($commit): array { + SpotMergeConflict::dispatchFor(Commit::factory()->create(['parent_id' => $commit->id])); + + return ['conflict' => false, 'ours' => 0, 'theirs' => 0]; + }]); + + resolve(CallQueuedHandler::class)->call(new FakeJob(), ['command' => serialize(Queue::pushed(SpotMergeConflict::class)->first())]); + + Queue::assertPushedTimes(SpotMergeConflict::class, 2); +}); + it('looks at the thread once a pending reply is approved', function (): void { Queue::fake([SpotMergeConflict::class]); $commit = Commit::factory()->create(); diff --git a/tests/Unit/MergeConflict.test.tsx b/tests/Unit/MergeConflict.test.tsx index a03a4cf4c..17401de93 100644 --- a/tests/Unit/MergeConflict.test.tsx +++ b/tests/Unit/MergeConflict.test.tsx @@ -118,6 +118,20 @@ test('names the side that won once the conflict closed, or a tie', () => { ); }); +test('shows zero counts once the conflict closed without votes', () => { + const html = render( + conflict({ + is_closed: true, + can_vote: false, + ours: { ...conflict().ours, votes: 0 }, + theirs: { ...conflict().theirs, votes: 0 }, + }), + ); + + assert.match(html, /Closed/); + assert.match(html, /@alice.*0% · 0.*@bob.*0% · 0/s); +}); + test('offers guests no buttons', () => { assert.doesNotMatch( render(conflict({ can_vote: false }), false), From dcc34900c513359fab3f4b2c827a07946e4f1b03 Mon Sep 17 00:00:00 2001 From: Houssem Eddine BENABDALLAH Date: Tue, 3 Nov 2026 11:55:20 +0100 Subject: [PATCH 6/7] fix: open a commit by its profile in the merge conflict vote tests --- tests/Feature/Controllers/FeatureControllerTest.php | 6 +++--- .../Controllers/MergeConflictVoteControllerTest.php | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/Feature/Controllers/FeatureControllerTest.php b/tests/Feature/Controllers/FeatureControllerTest.php index f1999711a..4515ecf2b 100644 --- a/tests/Feature/Controllers/FeatureControllerTest.php +++ b/tests/Feature/Controllers/FeatureControllerTest.php @@ -16,7 +16,7 @@ ->assertOk() ->assertInertia(fn ($page) => $page ->component('operations/features') - ->where('features', [['name' => 'translations', 'enabled' => true], ['name' => 'organization_commits', 'enabled' => false, ['name' => 'merge_conflicts', 'enabled' => true]])); + ->where('features', [['name' => 'translations', 'enabled' => true], ['name' => 'organization_commits', 'enabled' => false], ['name' => 'merge_conflicts', 'enabled' => true]])); }); it('hides the features from non admins', function (): void { @@ -33,7 +33,7 @@ $admin = User::factory()->create(['handle' => 'admin']); $this->actingAs($admin)->get(route('feature.index')) - ->assertInertia(fn ($page) => $page->where('features', [['name' => 'translations', 'enabled' => true], ['name' => 'organization_commits', 'enabled' => false, ['name' => 'merge_conflicts', 'enabled' => true]])); + ->assertInertia(fn ($page) => $page->where('features', [['name' => 'translations', 'enabled' => true], ['name' => 'organization_commits', 'enabled' => false], ['name' => 'merge_conflicts', 'enabled' => true]])); $this->fromRoute('feature.index') ->put(route('feature.update', 'translations'), ['enabled' => false]) @@ -42,7 +42,7 @@ expect(resolve(Features::class)->enabled(Feature::Translations))->toBeFalse(); $this->assertDatabaseHas('features', ['name' => 'translations', 'enabled' => false]); $this->get(route('feature.index')) - ->assertInertia(fn ($page) => $page->where('features', [['name' => 'translations', 'enabled' => false], ['name' => 'organization_commits', 'enabled' => false, ['name' => 'merge_conflicts', 'enabled' => true]])); + ->assertInertia(fn ($page) => $page->where('features', [['name' => 'translations', 'enabled' => false], ['name' => 'organization_commits', 'enabled' => false], ['name' => 'merge_conflicts', 'enabled' => true]])); $this->put(route('feature.update', 'translations'), ['enabled' => true])->assertRedirect(); diff --git a/tests/Feature/Controllers/MergeConflictVoteControllerTest.php b/tests/Feature/Controllers/MergeConflictVoteControllerTest.php index 23e1258da..993288e40 100644 --- a/tests/Feature/Controllers/MergeConflictVoteControllerTest.php +++ b/tests/Feature/Controllers/MergeConflictVoteControllerTest.php @@ -89,7 +89,7 @@ MergeConflictVote::factory()->for($mergeConflict)->count(2)->create(); $this->actingAs(User::factory()->create()) - ->get(route('commit.show', ['user' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) + ->get(route('commit.show', ['profile' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) ->assertOk() ->assertInertia(fn ($page) => $page ->where('mergeConflict.ours.id', $mergeConflict->ours_id) @@ -106,7 +106,7 @@ $mergeConflict = MergeConflict::factory()->closed()->create(); MergeConflictVote::factory()->for($mergeConflict)->count(2)->create(['side' => 'theirs']); - $this->get(route('commit.show', ['user' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) + $this->get(route('commit.show', ['profile' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) ->assertOk() ->assertInertia(fn ($page) => $page ->where('mergeConflict.is_closed', true) @@ -120,7 +120,7 @@ $reader->blocking()->attach($mergeConflict->theirs->user); $this->actingAs($reader) - ->get(route('commit.show', ['user' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) + ->get(route('commit.show', ['profile' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) ->assertInertia(fn ($page) => $page->where('mergeConflict', null)); }); @@ -128,6 +128,6 @@ $mergeConflict = MergeConflict::factory()->create(); resolve(Features::class)->update(Feature::MergeConflicts, false); - $this->get(route('commit.show', ['user' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) + $this->get(route('commit.show', ['profile' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) ->assertInertia(fn ($page) => $page->where('mergeConflict', null)); }); From a4af4707ddd9b9ed2748558169e236fafd1c46c9 Mon Sep 17 00:00:00 2001 From: Houssem Eddine BENABDALLAH Date: Tue, 3 Nov 2026 15:40:30 +0100 Subject: [PATCH 7/7] fix: show a merge conflict side posted as an organization as the organization --- app/Actions/CreateMergeConflict.php | 4 +-- app/Queries/MergeConflictQuery.php | 11 +++--- resources/js/components/merge-conflict.tsx | 12 +++---- resources/js/types/commit.ts | 2 +- .../MergeConflictVoteControllerTest.php | 17 ++++++++- .../Unit/Actions/CreateMergeConflictTest.php | 12 +++++++ tests/Unit/MergeConflict.test.tsx | 35 ++++++++++++++++--- 7 files changed, 74 insertions(+), 19 deletions(-) diff --git a/app/Actions/CreateMergeConflict.php b/app/Actions/CreateMergeConflict.php index 2a87c0393..d110a5c17 100644 --- a/app/Actions/CreateMergeConflict.php +++ b/app/Actions/CreateMergeConflict.php @@ -61,7 +61,7 @@ public function handle(Commit $commit): ?MergeConflict 'post' => mb_substr($commit->body, 0, self::MaxReplyLength), 'replies' => $replies->values()->map(fn (Commit $reply, int $index): array => [ 'number' => $index + 1, - 'author' => '@'.$reply->user?->handle, + 'author' => '@'.$reply->author?->handle, 'body' => mb_substr($reply->body, 0, self::MaxReplyLength), ]), ])->toJson()); @@ -113,7 +113,7 @@ private function replies(Commit $commit): Collection ->whereNotNull('user_id') ->where('body', '!=', '') ->when($tom instanceof User, fn ($query) => $query->where('user_id', '!=', $tom?->id)) - ->with('user') + ->with('author') ->oldest() ->oldest('id') ->limit(self::MaxReplies) diff --git a/app/Queries/MergeConflictQuery.php b/app/Queries/MergeConflictQuery.php index 0ad8852cd..8e4d2f76d 100644 --- a/app/Queries/MergeConflictQuery.php +++ b/app/Queries/MergeConflictQuery.php @@ -8,6 +8,7 @@ use App\Enums\MergeConflictSide; use App\Models\Commit; use App\Models\MergeConflict; +use App\Models\Profile; use App\Models\User; use App\Services\Features; use Illuminate\Support\Str; @@ -25,7 +26,7 @@ public function __construct(private Features $features) * The merge conflict in the replies to a commit, as long as the reader can see both sides. Like a poll, the votes * are only counted for those who picked a side, the two authors, and everyone once voting has closed. * - * @return array{id: string, closes_at: string, is_closed: bool, side: string|null, can_vote: bool, ours: array{id: string, body: string, user: User|null, votes: int|null}, theirs: array{id: string, body: string, user: User|null, votes: int|null}}|null + * @return array{id: string, closes_at: string, is_closed: bool, side: string|null, can_vote: bool, ours: array{id: string, body: string, author: Profile|null, votes: int|null}, theirs: array{id: string, body: string, author: Profile|null, votes: int|null}}|null */ public function get(Commit $commit, ?User $viewer): ?array { @@ -39,7 +40,7 @@ public function get(Commit $commit, ?User $viewer): ?array ->visibleTo($viewer) ->withoutMutedAuthorsFor($viewer) ->whereKey([$mergeConflict->ours_id, $mergeConflict->theirs_id]) - ->with('user') + ->with('author') ->get() ->keyBy('id'); @@ -70,14 +71,16 @@ public function get(Commit $commit, ?User $viewer): ?array } /** - * @return array{id: string, body: string, user: User|null, votes: int|null} + * Who each side is from, as readers see it: an organization's reply shows the organization, not who posted it. + * + * @return array{id: string, body: string, author: Profile|null, votes: int|null} */ private function side(Commit $reply, mixed $votes): array { return [ 'id' => $reply->id, 'body' => Str::limit($reply->body, self::ExcerptLength), - 'user' => $reply->user, + 'author' => $reply->author, 'votes' => is_numeric($votes) ? (int) $votes : null, ]; } diff --git a/resources/js/components/merge-conflict.tsx b/resources/js/components/merge-conflict.tsx index 55f677156..8836dff7a 100644 --- a/resources/js/components/merge-conflict.tsx +++ b/resources/js/components/merge-conflict.tsx @@ -3,9 +3,9 @@ import { Check, GitMerge } from 'lucide-react'; import { useEffect, useState } from 'react'; import { toast } from 'sonner'; import MergeConflictVoteController from '@/actions/App/Http/Controllers/MergeConflictVoteController'; +import { AuthorAvatar } from '@/components/author-avatar'; import { timeLeft } from '@/components/commit-poll'; import { Button } from '@/components/ui/button'; -import { UserAvatar } from '@/components/user-avatar'; import { useCommitNotFound } from '@/hooks/use-deleted-resources'; import { useNow } from '@/hooks/use-now'; import { formatExactCount } from '@/lib/numbers'; @@ -22,7 +22,7 @@ type Side = 'ours' | 'theirs'; const failedVoteMessage = 'Could not record your pick. Try again.'; function handleOf(side: MergeConflictSide): string { - return side.user === null ? 'someone' : `@${side.user.handle}`; + return side.author === null ? 'someone' : `@${side.author.handle}`; } function winnerOf(mergeConflict: MergeConflictData): Side | null { @@ -65,13 +65,13 @@ function ConflictSide({ )} > {marker} - {side.user !== null && ( + {side.author !== null && ( - diff --git a/resources/js/types/commit.ts b/resources/js/types/commit.ts index 39449e825..b490c56e9 100644 --- a/resources/js/types/commit.ts +++ b/resources/js/types/commit.ts @@ -43,7 +43,7 @@ export type Poll = { export type MergeConflictSide = { id: string; body: string; - user: User | null; + author: CommitAuthor | null; votes: number | null; }; diff --git a/tests/Feature/Controllers/MergeConflictVoteControllerTest.php b/tests/Feature/Controllers/MergeConflictVoteControllerTest.php index 993288e40..d88cb2edf 100644 --- a/tests/Feature/Controllers/MergeConflictVoteControllerTest.php +++ b/tests/Feature/Controllers/MergeConflictVoteControllerTest.php @@ -5,6 +5,7 @@ use App\Enums\Feature; use App\Models\MergeConflict; use App\Models\MergeConflictVote; +use App\Models\Organization; use App\Models\User; use App\Services\Features; @@ -94,7 +95,8 @@ ->assertInertia(fn ($page) => $page ->where('mergeConflict.ours.id', $mergeConflict->ours_id) ->where('mergeConflict.ours.body', $mergeConflict->ours->body) - ->where('mergeConflict.ours.user.handle', $mergeConflict->ours->user->handle) + ->where('mergeConflict.ours.author.type', 'user') + ->where('mergeConflict.ours.author.handle', $mergeConflict->ours->user->handle) ->where('mergeConflict.theirs.id', $mergeConflict->theirs_id) ->where('mergeConflict.ours.votes', null) ->where('mergeConflict.side', null) @@ -102,6 +104,19 @@ ->where('mergeConflict.is_closed', false)); }); +it('shows a side posted as an organization as the organization, not who posted it', function (): void { + $organization = Organization::factory()->create(['handle' => 'laravel']); + $mergeConflict = MergeConflict::factory()->create(); + $mergeConflict->theirs->update(['author_id' => $organization->id]); + + $this->actingAs(User::factory()->create()) + ->get(route('commit.show', ['profile' => $mergeConflict->commit->user, 'commit' => $mergeConflict->commit])) + ->assertOk() + ->assertInertia(fn ($page) => $page + ->where('mergeConflict.theirs.author.type', 'organization') + ->where('mergeConflict.theirs.author.handle', 'laravel')); +}); + it('shows everyone the final count once the conflict closed', function (): void { $mergeConflict = MergeConflict::factory()->closed()->create(); MergeConflictVote::factory()->for($mergeConflict)->count(2)->create(['side' => 'theirs']); diff --git a/tests/Unit/Actions/CreateMergeConflictTest.php b/tests/Unit/Actions/CreateMergeConflictTest.php index 3c1ca5938..0724a03a6 100644 --- a/tests/Unit/Actions/CreateMergeConflictTest.php +++ b/tests/Unit/Actions/CreateMergeConflictTest.php @@ -8,6 +8,7 @@ use App\Enums\Feature; use App\Models\Commit; use App\Models\MergeConflict; +use App\Models\Organization; use App\Models\User; use App\Services\Features; use Laravel\Ai\Prompts\AgentPrompt; @@ -41,6 +42,17 @@ function repliesTo(Commit $commit, string ...$bodies): array && str_contains($prompt->prompt, '"body":"Spaces, tabs are wrong."')); }); +it('names a reply posted as an organization by the organization when asking Tom', function (): void { + ConflictSpotter::fake([['conflict' => false, 'ours' => 0, 'theirs' => 0]])->preventStrayPrompts(); + $commit = Commit::factory()->create(); + [, $reply] = repliesTo($commit, 'Tabs, always.', 'Spaces, tabs are wrong.'); + $reply->update(['author_id' => Organization::factory()->create(['handle' => 'laravel'])->id]); + + resolve(CreateMergeConflict::class)->handle($commit); + + ConflictSpotter::assertPrompted(fn (AgentPrompt $prompt): bool => str_contains($prompt->prompt, '"author":"@laravel"')); +}); + it('puts the earlier reply on our side whichever way Tom names them', function (): void { ConflictSpotter::fake([['conflict' => true, 'ours' => 2, 'theirs' => 1]]); $commit = Commit::factory()->create(); diff --git a/tests/Unit/MergeConflict.test.tsx b/tests/Unit/MergeConflict.test.tsx index 17401de93..e12042ab7 100644 --- a/tests/Unit/MergeConflict.test.tsx +++ b/tests/Unit/MergeConflict.test.tsx @@ -7,20 +7,21 @@ import { test } from 'vite-plus/test'; import { MergeConflict } from '../../resources/js/components/merge-conflict'; import { TooltipProvider } from '../../resources/js/components/ui/tooltip'; import type { + CommitAuthor, MergeConflict as MergeConflictData, - User, } from '../../resources/js/types'; const commit = { id: '019a2b3c-0000-7000-8000-000000000001' }; -function author(handle: string): User { +function author(handle: string): CommitAuthor { return { + type: 'user', id: handle, handle, name: handle, avatar_url: null, lane: 0, - } as User; + } as CommitAuthor; } function conflict( @@ -35,13 +36,13 @@ function conflict( ours: { id: 'ours', body: 'Tabs, always.', - user: author('alice'), + author: author('alice'), votes: null, }, theirs: { id: 'theirs', body: 'Spaces. Tabs are wrong.', - user: author('bob'), + author: author('bob'), votes: null, }, ...overrides, @@ -138,3 +139,27 @@ test('offers guests no buttons', () => { /Accept @/, ); }); + +test('shows a side posted as an organization as the organization, square and linked to its page', () => { + const html = render( + conflict({ + theirs: { + id: 'theirs', + body: 'Spaces. Tabs are wrong.', + author: { + type: 'organization', + id: 'laravel', + handle: 'laravel', + name: 'Laravel', + avatar_url: + 'https://avatars.githubusercontent.com/u/958072', + lane: null, + } as CommitAuthor, + votes: null, + }, + }), + ); + + assert.match(html, /href="\/@laravel"[^>]*>.*rounded-md.*@laravel/s); + assert.match(html, /Accept @laravel/); +});