Review recent changes and documentation

[Chris0Jeky]

No description provided.

[Copilot]

Pull Request Overview

This PR addresses backend test infrastructure issues and implements critical frontend card management functionality. The changes fix all 9 failing backend tests by replacing reflection-based test setup with proper domain encapsulation patterns, and add a comprehensive CardModal component that enables full CRUD operations for cards.

Key Changes:

• Fixed all 9 failing backend tests by adding InternalsVisibleTo and enhanced TestDataBuilder helper methods
• Implemented CardModal component with full card editing, label management, due date handling, block/unblock functionality, and deletion
• Added updateCard and deleteCard actions to the Pinia store for centralized state management

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
backend/src/Taskdeck.Domain/Taskdeck.Domain.csproj	Added InternalsVisibleTo attribute to expose internal domain methods to tests
backend/src/Taskdeck.Domain/Entities/CardLabel.cs	Changed navigation property setters from private to internal for test access
backend/tests/Taskdeck.Application.Tests/TestUtilities/TestDataBuilder.cs	Added 4 helper methods to properly set up complex test scenarios without reflection
backend/tests/Taskdeck.Application.Tests/Services/CardServiceTests.cs	Updated 6 tests to use new helper methods instead of reflection
backend/tests/Taskdeck.Application.Tests/Services/ColumnServiceTests.cs	Updated 1 test to use CreateColumnWithCards helper
backend/tests/Taskdeck.Application.Tests/Services/BoardServiceTests.cs	Updated 2 tests to use CreateBoardWithColumns helper
frontend/taskdeck-web/src/store/boardStore.ts	Added updateCard and deleteCard store actions with proper state synchronization
frontend/taskdeck-web/src/components/board/CardModal.vue	New comprehensive modal component for editing cards with validation and full feature support
frontend/taskdeck-web/src/components/board/CardItem.vue	Added click event emit to enable opening the card modal
frontend/taskdeck-web/src/components/board/ColumnLane.vue	Integrated CardModal with click handler and state management
SESSION_SUMMARY_2025-11-18_Implementation.md	Comprehensive session documentation detailing all changes and decisions
IMPLEMENTATION_STATUS.md	Updated to reflect completed test fixes and CardModal implementation

---

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

[Copilot]

The comment states "Fixed 4 tests" but then lists 6 test line numbers (123, 168, 216, 245, 521, 548). This is inconsistent.

Correction: Should say "Fixed 6 tests" to match the listed line numbers.

[Copilot]

The dueDate field handling has a bug that prevents clearing the due date.

Issue: On line 63, when dueDate.value is an empty string (after clicking "Clear"), the ternary expression evaluates to null. However, the backend's Card.Update() method only updates DueDate when dueDate.HasValue is true (line 64-65 in Card.cs). This means sending null will NOT clear an existing due date.

Solution: The logic should distinguish between "don't update" (undefined) and "clear the value" (null). Consider:

dueDate: dueDate.value 
  ? new Date(dueDate.value).toISOString() 
  : (dueDate.value === '' && props.card.dueDate ? null : undefined)

Or better yet, track whether the due date field was actually changed, similar to how title and description are handled.

Suggested change

	dueDate: dueDate.value ? new Date(dueDate.value).toISOString() : null,
	dueDate:
	dueDate.value === (props.card.dueDate ? new Date(props.card.dueDate).toISOString().split('T')[0] : '')
	? undefined
	: dueDate.value
	? new Date(dueDate.value).toISOString()
	: (props.card.dueDate ? null : undefined),

[Copilot]

Inconsistent conditional update logic. The title and description fields are only sent when they differ from the original values, but labelIds is always sent (line 66) even if unchanged. This could cause unnecessary database writes.

Recommendation: Apply the same conditional logic:

labelIds: JSON.stringify(selectedLabelIds.value.sort()) !== JSON.stringify(props.card.labels.map(l => l.id).sort())
  ? selectedLabelIds.value
  : null

This ensures consistency with how other fields are handled and avoids unnecessary label updates.

Suggested change

	labelIds: selectedLabelIds.value
	labelIds: JSON.stringify(selectedLabelIds.value.slice().sort()) !== JSON.stringify(props.card.labels.map(l => l.id).slice().sort())
	? selectedLabelIds.value
	: null

[Copilot]

The blockReason field is always sent when isBlocked is true (line 65), even if it hasn't changed. This differs from how title and description are conditionally sent only when changed.

Recommendation: Use conditional logic:

blockReason: isBlocked.value 
  ? (blockReason.value !== props.card.blockReason ? blockReason.value : null)
  : null

This ensures consistency and avoids unnecessary updates.

Suggested change

	blockReason: isBlocked.value ? blockReason.value : null,
	blockReason: isBlocked.value
	? (blockReason.value !== props.card.blockReason ? blockReason.value : null)
	: null,

[Copilot]

[nitpick] The test passes Guid.NewGuid() as the cardId to CreateCardLabelWithLabel, which creates a CardLabel with an ID that doesn't match the card it's being added to (lines 171-175). While this works in mocked tests, it creates semantically incorrect test data.

Issue: The CardLabel will have a different CardId than the card object's actual ID.

Recommendation: Use the card's ID after creating it:

var card = TestDataBuilder.CreateCard(board.Id, column.Id, dto.Title);
var cardLabel1 = TestDataBuilder.CreateCardLabelWithLabel(card.Id, label1);
var cardLabel2 = TestDataBuilder.CreateCardLabelWithLabel(card.Id, label2);
card.AddLabel(cardLabel1);
card.AddLabel(cardLabel2);
return card;

Or update CreateCardWithLabels to handle this internally by updating the CardLabel's CardId.

[Copilot]

[nitpick] The test passes Guid.NewGuid() as the cardId to CreateCardLabelWithLabel, which creates a CardLabel with an ID that doesn't match the card it's being added to (lines 218-222). While this works in mocked tests, it creates semantically incorrect test data.

Issue: The CardLabel will have a different CardId than the card object's actual ID.

Recommendation: Use the card's ID after creating it:

var card = TestDataBuilder.CreateCard(board.Id, column.Id, dto.Title);
var cardLabel = TestDataBuilder.CreateCardLabelWithLabel(card.Id, validLabel);
card.AddLabel(cardLabel);
return card;

Or update CreateCardWithLabels to handle this internally.

[Copilot]

The close button (X) lacks an accessible label for screen readers. While the SVG is visible to sighted users, screen reader users won't know what the button does.

Recommendation: Add an aria-label attribute:

<button
  @click="handleClose"
  aria-label="Close dialog"
  class="text-gray-400 hover:text-gray-600 transition-colors"
>

Suggested change

	@click="handleClose"
	@click="handleClose"
	aria-label="Close dialog"

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Persist block reason when status unchanged

Saving a card that is already blocked after editing its block reason sends isBlocked: null while still sending the new blockReason (lines 64–65). CardService.UpdateCardAsync only applies block/unblock logic when IsBlocked is provided, so the update is skipped and the new reason is lost unless the user toggles the checkbox off/on. Blocked cards therefore cannot have their reason corrected or updated without also changing their blocked state.

Useful? React with 👍 / 👎.