-
-
Notifications
You must be signed in to change notification settings - Fork 34
Feature/concurrent card dragging #64
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
Conversation
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 concurrent card dragging support by fixing parameter order mismatches between JavaScript and PHP, adding pessimistic locking to prevent race conditions, and implementing automatic retry logic for conflict resolution.
- Fixed critical parameter order bug where JavaScript and PHP were passing
afterCardIdandbeforeCardIdin reversed order - Implemented pessimistic locking with
lockForUpdate()to prevent concurrent modification conflicts - Added retry mechanism with exponential backoff for handling rare duplicate position conflicts
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| resources/js/flowforge.js | Fixed parameter order in moveCard call to match PHP expectations (afterCardId, beforeCardId) |
| resources/dist/flowforge.js | Compiled JavaScript with corrected parameter order |
| src/Concerns/InteractsWithBoard.php | Added pessimistic locking, retry logic, and corrected parameter usage in position calculations |
| src/FlowforgeServiceProvider.php | Registered new DiagnosePositionsCommand |
| src/Exceptions/MaxRetriesExceededException.php | New exception for retry exhaustion scenarios |
| src/Commands/DiagnosePositionsCommand.php | New diagnostic command to detect position inversions, duplicates, and collation issues |
| database/migrations/add_unique_position_constraint.php.stub | Migration template for adding unique constraint on position columns |
| tests/Feature/PositionInversionReproductionTest.php | Tests reproducing position inversion bugs from various scenarios |
| tests/Feature/PerformanceRegressionTest.php | Performance benchmarks for position operations at scale |
| tests/Feature/ParameterOrderMutationTest.php | Tests validating correct parameter order and defining shared helper function |
| tests/Feature/ParameterCombinationTest.php | Tests exploring all parameter combinations to verify fix |
| tests/Feature/JavaScriptPhpParameterFlowTest.php | Tests validating exact JavaScript-to-PHP parameter flow |
| tests/Feature/DragDropFunctionalityTest.php | Updated existing tests with corrected expectations and added stress tests |
| tests/Feature/ConcurrentOperationStressTest.php | Stress tests simulating concurrent operations |
| tests/Feature/CharacterSpaceExhaustionTest.php | Tests for position string growth and character space limits |
Comments suppressed due to low confidence (1)
src/Concerns/InteractsWithBoard.php:374
- The
calculatePositionBetweenCardsmethod appears to be dead code that is no longer used after the refactoring. It has been replaced bycalculatePositionBetweenLockedCardswhich is called within transactions. Consider removing this method to reduce code maintenance burden and confusion.
protected function calculatePositionBetweenCards(
?string $afterCardId = null,
?string $beforeCardId = null,
?string $columnId = null
): string {
if (! $afterCardId && ! $beforeCardId && $columnId) {
return $this->getBoardPositionInColumn($columnId, 'bottom');
}
$query = $this->getBoard()->getQuery();
if (! $query) {
return Rank::forEmptySequence()->get();
}
$positionField = $this->getBoard()->getPositionIdentifierAttribute();
$beforeCard = $beforeCardId ? (clone $query)->find($beforeCardId) : null;
$beforePos = $beforeCard?->getAttribute($positionField);
$afterCard = $afterCardId ? (clone $query)->find($afterCardId) : null;
$afterPos = $afterCard?->getAttribute($positionField);
if ($beforePos && $afterPos && is_string($beforePos) && is_string($afterPos)) {
return Rank::betweenRanks(Rank::fromString($afterPos), Rank::fromString($beforePos))->get();
}
if ($beforePos && is_string($beforePos)) {
return Rank::before(Rank::fromString($beforePos))->get();
}
if ($afterPos && is_string($afterPos)) {
return Rank::after(Rank::fromString($afterPos))->get();
}
return Rank::forEmptySequence()->get();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.