Skip to content

Conversation

@ManukMinasyan
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings December 13, 2025 20:36
@ManukMinasyan ManukMinasyan merged commit 9ec2bc2 into 2.x Dec 13, 2025
7 checks passed
Copy link

Copilot AI left a 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 pull request fixes a record binding issue in BoardResourcePage when child classes use the InteractsWithRecord trait, specifically addressing GitHub issue #37. The fix adds proper action resolution routing to distinguish between board card actions and resource record actions.

Key Changes:

  • Introduces a new resolveBoardAction method to properly handle board card actions with record resolution
  • Overrides resolveActions in BoardResourcePage to intercept and route board-specific actions before delegating to Filament's standard action resolution
  • Adds test fixtures (TestResource and TestBoardResourcePage) that replicate the issue scenario where a project has many tasks

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/BoardResourcePage.php Added resolveActions override to detect and route board card actions vs resource actions, enabling proper record binding for child classes using InteractsWithRecord
src/Concerns/InteractsWithBoard.php Added resolveBoardAction method to resolve board card actions and set the correct record context, similar to how table actions are resolved
tests/Fixtures/TestResource.php New minimal Filament resource fixture for testing the board resource page with InteractsWithRecord scenario
tests/Fixtures/TestBoardResourcePage.php New test fixture demonstrating a BoardResourcePage that uses InteractsWithRecord to scope tasks to a specific project (GitHub issue #37 scenario)
phpunit.xml.dist Disabled failOnWarning and commented out coverage configuration for test execution flexibility

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +290 to +321
* Resolve a board action (similar to resolveTableAction).
*/
protected function resolveBoardAction(array $action, array $parentActions): ?Action
{
$resolvedAction = null;

if (count($parentActions)) {
$parentAction = end($parentActions);
$resolvedAction = $parentAction->getModalAction($action['name']);
} else {
$resolvedAction = $this->cachedActions[$action['name']] ?? null;
}

if (! $resolvedAction) {
return null;
}

$recordKey = $action['context']['recordKey'] ?? $action['arguments']['recordKey'] ?? null;

if (filled($recordKey)) {
$board = $this->getBoard();
$query = $board->getQuery();

if ($query) {
$record = (clone $query)->find($recordKey);
$resolvedAction->record($record);
}
}

return $resolvedAction;
}

Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

The resolveBoardAction method is defined in the InteractsWithBoard trait, but it's being called in BoardResourcePage.resolveActions(). Since BoardResourcePage uses the BaseBoard trait which includes InteractsWithBoard, this method will be available. However, to improve code organization and follow the same pattern as resolveTableAction and resolveSchemaComponentAction (which come from Filament's InteractsWithTable and InteractsWithForms traits respectively), consider whether this method should remain in InteractsWithBoard or be moved to BoardResourcePage where it's actually called.

Suggested change
* Resolve a board action (similar to resolveTableAction).
*/
protected function resolveBoardAction(array $action, array $parentActions): ?Action
{
$resolvedAction = null;
if (count($parentActions)) {
$parentAction = end($parentActions);
$resolvedAction = $parentAction->getModalAction($action['name']);
} else {
$resolvedAction = $this->cachedActions[$action['name']] ?? null;
}
if (! $resolvedAction) {
return null;
}
$recordKey = $action['context']['recordKey'] ?? $action['arguments']['recordKey'] ?? null;
if (filled($recordKey)) {
$board = $this->getBoard();
$query = $board->getQuery();
if ($query) {
$record = (clone $query)->find($recordKey);
$resolvedAction->record($record);
}
}
return $resolvedAction;
}
*
*/

Copilot uses AI. Check for mistakes.

foreach ($actions as $actionNestingIndex => $action) {
if (blank($action['name'] ?? null)) {
throw new \Filament\Actions\Exceptions\ActionNotResolvableException('An action tried to resolve without a name.');
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

The code uses a fully qualified exception class name instead of relying on the imported ActionNotResolvableException at line 9. For consistency, use the imported class name instead of the fully qualified name.

Suggested change
throw new \Filament\Actions\Exceptions\ActionNotResolvableException('An action tried to resolve without a name.');
throw new ActionNotResolvableException('An action tried to resolve without a name.');

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +24
<?php

declare(strict_types=1);

namespace Relaticle\Flowforge\Tests\Fixtures;

use Filament\Resources\Resource;

/**
* Minimal test resource for TestBoardResourcePage.
*/
class TestResource extends Resource
{
protected static ?string $model = Project::class;

protected static ?string $slug = 'test-projects';

public static function getPages(): array
{
return [
'board' => TestBoardResourcePage::route('/{record}/board'),
];
}
}
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

These test fixtures (TestResource and TestBoardResourcePage) appear to be created to address GitHub issue 37, but there are no corresponding test files that use these fixtures to verify the fix. Without actual tests exercising these fixtures, it's unclear whether the implementation correctly solves the issue. Consider adding test cases that use these fixtures to verify the record binding functionality works correctly when InteractsWithRecord trait is used with BoardResourcePage.

Copilot uses AI. Check for mistakes.
// This detection happens BEFORE schema/table action detection
$recordKey = $action['context']['recordKey'] ?? null;
$columnId = $action['arguments']['column'] ?? null;

Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

The logic for detecting board card actions checks if recordKey is present AND columnId is blank. However, this logic may not handle all edge cases correctly. For instance, if an action has both recordKey and column in different contexts (e.g., a card action within a column), the condition might not work as expected. Consider adding clearer documentation about when actions have recordKey vs column, or add validation to ensure these are mutually exclusive when expected.

Suggested change
// Validation: recordKey and columnId should be mutually exclusive.
// If both are present, this is an invalid/ambiguous action context.
if (filled($recordKey) && filled($columnId)) {
throw new \Filament\Actions\Exceptions\ActionNotResolvableException(
"Ambiguous board action: both 'recordKey' and 'column' are present. " .
"Actions must have either 'recordKey' (for card actions) or 'column' (for column actions), not both."
);
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants