diff --git a/.squad/agents/aragorn/history.md b/.squad/agents/aragorn/history.md index 690d7225..6955f105 100644 --- a/.squad/agents/aragorn/history.md +++ b/.squad/agents/aragorn/history.md @@ -1346,6 +1346,7 @@ Posted delivery summary comment on issue #291 covering all 5 shipped items: **Copilot Review:** 3 comments; no flagged bugs or security issues **Content:** Bundles 3 concerns: + 1. **Merge conflict resolution** — local `dev` vs `origin/dev` (commit 7e15cdd); correctly retained board automation option IDs 2. **DevOps/CI fix** — switches `GITHUB_TOKEN` → `GH_PROJECT_TOKEN`, updates stale project board option IDs (IN_SPRINT, IN_REVIEW, RELEASED) 3. **UX bug fix** — Edit.razor redirect when `GetBlogPostByIdQuery` returns `Result.Ok(null)` + bUnit regression test @@ -1359,6 +1360,7 @@ Posted delivery summary comment on issue #291 covering all 5 shipped items: **Content:** Only the Edit.razor redirect + bUnit test (identical to PR #306) **Problem:** Duplicate of PR #306 UI changes: + - Commit hashes differ (8c7b15f vs 8cf7981) but content is identical - Based on old commit; separate merge creates conflicts - Prevents clean history and wastes reviewer time @@ -1373,11 +1375,15 @@ Posted delivery summary comment on issue #291 covering all 5 shipped items: ### Learnings -**Multiple squad-authored PRs from the same session can overlap.** When Boromir resolved the merge conflict for #305, the merge reintroduced Edit.razor redirect fix (from upstream PR #304 resolution). Boromir then filed a separate PR #308 for the same UI fix, not realizing it was already in #306. This is a normal situation — the remedy is triage-time duplicate detection + closure. **Do not suppress triage when multiple PRs land; always check for overlap.** +**Multiple squad-authored PRs from the same session can overlap.** When Boromir resolved the merge conflict for #305, the merge reintroduced Edit.razor redirect fix +(from upstream PR #304 resolution). Boromir then filed a separate PR #308 for the same UI fix, not realizing it was already in #306. +This is a normal situation — the remedy is triage-time duplicate detection + closure. **Do not suppress triage when multiple PRs land; always check for overlap.** **Merge conflicts can carry forward bugfixes from upstream.** When resolving `local dev` vs `origin/dev`, the merge commit inherited the Edit.razor UX fix that was in the upstream branch. This is correct — don't discard upstream bugfixes during conflict resolution. However, document it clearly in the PR body (as Boromir did) so reviewers understand what's being carried forward. -**`squad:member` labels require GitHub API token with `read:org` scope.** The `gh` CLI edit command failed because the token was scoped to `['read:user', 'repo', 'user:email', 'workflow']` but `--add-label` requires `read:org` and `read:discussion` scopes. Workaround: post triage comments instead, and let squad members apply labels manually or via the GitHub UI. Consider requesting a broader token for future triage work. +**`squad:member` labels require GitHub API token with `read:org` scope.** The `gh` CLI edit command failed because the token was scoped to +`['read:user', 'repo', 'user:email', 'workflow']` but `--add-label` requires `read:org` and `read:discussion` scopes. +Workaround: post triage comments instead, and let squad members apply labels manually or via the GitHub UI. Consider requesting a broader token for future triage work. ## 2026-07-12 — PR #310 Review: fix(ui): refresh Edit page loading state on post-ID changes @@ -1414,5 +1420,38 @@ Posted delivery summary comment on issue #291 covering all 5 shipped items: **GitHub cannot self-approve or self-request-changes.** `gh pr review --request-changes` fails when the reviewer is the PR author. Post a review comment instead. This is a known squad limitation. -**`try/finally` is necessary but not sufficient for Blazor parameter reload.** A loading flag reset via `try/finally` correctly gates the loading indicator, but state fields like `_model`, `_error`, and `_concurrencyError` that survive from a previous render cycle must also be explicitly cleared at the top of `OnParametersSetAsync`. Copilot correctly flagged this pattern; it should be treated as a standard rule for all Blazor components using `OnParametersSetAsync` with cached state fields. +**`try/finally` is necessary but not sufficient for Blazor parameter reload.** A loading flag reset via `try/finally` correctly gates the loading indicator, +but state fields like `_model`, `_error`, and `_concurrencyError` that survive from a previous render cycle must also be explicitly cleared at the top of `OnParametersSetAsync`. +Copilot correctly flagged this pattern; it should be treated as a standard rule for all Blazor components using `OnParametersSetAsync` with cached state fields. + +--- + +## PR #313 Review — fix(blogposts): align author claims, publish checkbox, and seed schema + +**Date:** 2026-05-12 +**PR Author:** Boromir (DevOps/Infra) + Copilot +**Verdict:** ✅ APPROVED (gate comment posted; GitHub self-approval blocked — comment used instead) + +### What Was Reviewed + +- **Author claim extraction:** Aligned to `ClaimTypes.NameIdentifier` with `sub` fallback — correct and consistent with `Program.cs` line 198. +- **IsPublished tri-state on EditBlogPostCommand:** `bool? IsPublished = null` correctly distinguishes "no change intent" from explicit true/false. Clean CQRS design. +- **Domain model usage:** `post.Publish()` / `post.Unpublish()` called in handlers — no domain logic leaked. +- **Seed schema fix:** `Author` as embedded `BsonDocument` matches Mongo EF owned-type mapping. +- **Test gate:** All 375 tests pass (Domain, Architecture, Web.Tests, Web.Tests.Bunit, Web.Tests.Integration, AppHost.Tests). + +### Copilot Comments Disposition + +| # | Comment | Disposition | +|---|---------|-------------| +| 1 | Fallback inconsistency Create vs Edit (NameIdentifier → sub → Identity.Name → "dev-user") | Follow-up issue — production Auth0 always provides NameIdentifier; dev-only concern | +| 2 | Dead code: `IsNullOrWhiteSpace` block unreachable after `?? "dev-user"` | Follow-up cleanup — no functional regression | +| 3 | Edit only falls back to empty string; Create may persist "dev-user" | Same as comment 1 — follow-up | + +### Follow-Up Recommended + +Extract a shared `UserIdClaimsHelper` to unify AuthorId extraction across Create/Edit Razor pages. Assign to **Legolas**. + +### Learnings +GitHub prevents self-approval on PRs. Use `gh pr comment` as the gate signal when `gh pr review --approve` is blocked. diff --git a/.squad/agents/gimli/history.md b/.squad/agents/gimli/history.md index d02184f2..258ce5a7 100644 --- a/.squad/agents/gimli/history.md +++ b/.squad/agents/gimli/history.md @@ -1006,3 +1006,39 @@ when a post ID was not found. The fix redirects to `/blog` instead. 2. **The stale-content test pattern is behavior-first** — asserting `Markup.Should().Contain("Second Post Title")` and `Markup.Should().NotContain("First Post Title")` verifies what the user sees, not internal state. Survives any refactor of `_model` or `_isLoading` that preserves visible output. 3. **`Render(...)` in bUnit 2.x waits for async lifecycle** — `OnParametersSetAsync` completes before `Render` returns, so synchronous markup assertions after `Render` are safe; `WaitForAssertion` is not needed here. 4. **View tool can show stale snapshots** — always verify actual file content with `bash`/`cat` before concluding a file needs editing. + +--- + +## Test Runs + +### 2025 — PR #313: fix(blogposts): align author claims, publish checkbox, and seed schema + +**Requested by:** Ralph (via coordinator) + +#### Task + +Run the full local test suite against PR #313 changes and report results. + +#### Test Results + +| Suite | Passed | Failed | Skipped | Duration | +|-------|--------|--------|---------|----------| +| Architecture.Tests | 16 | 0 | 0 | 93 ms | +| Domain.Tests | 42 | 0 | 0 | 91 ms | +| Web.Tests | 158 | 0 | 0 | 180 ms | +| Web.Tests.Bunit | 92 | 0 | 0 | 514 ms | +| **Total** | **308** | **0** | **0** | | + +✅ **All 308 tests pass. Zero failures. Zero skips.** + +#### Failures + +None. No failures to triage. + +#### Production Code Issues Flagged + +None. No production code issues surfaced by the test suite. + +#### Decisions File + +Not created — no failures, no handoff required. diff --git a/.squad/decisions.md b/.squad/decisions.md index 91b95d23..02aafb69 100644 --- a/.squad/decisions.md +++ b/.squad/decisions.md @@ -2433,3 +2433,383 @@ Feature branch commits **must never include `.squad/` files**. Stage explicitly **✅ Implemented** — Both regressions fixed in PR #295. Process applied and documented. --- + +--- + +### 32. OnParametersSetAsync State Reset Pattern for Parameterized Blazor Pages + +**Author:** Aragorn (Lead / Architect) +**Date:** 2026-05-16 +**Related Issue:** #312 ([Sprint 19] Fix Blog Posts page errors) +**Status:** Documented — Pattern applied to `Edit.razor` in PRs #307, #309, #310 + +#### Context + +Comprehensive diagnosis of Blog Posts page errors (Issue #312) revealed that `Edit.razor` had three related UI bugs caused by missing state reset in `OnParametersSetAsync`. The bugs were already fixed on `origin/dev` (PRs #307, #309, #310), but the pattern violation is worth codifying as a team decision to prevent recurrence in future parameterized pages. + +#### Decision + +**All Blazor components that use `OnParametersSetAsync` with route parameters MUST follow this pattern:** + +```csharp +protected override async Task OnParametersSetAsync() +{ + // 1. Reset ALL private state fields before async work + _model = null; + _error = null; + _isLoading = true; + // reset any other state fields... + + try + { + // 2. Perform async work + var result = await Sender.Send(new MyQuery(Id)); + // ... handle result + } + finally + { + // 3. ALWAYS clear loading state, even on navigation-away + _isLoading = false; + } +} +``` + +**Required conditions:** + +- Any component handling a parameterized route (e.g., `/blog/edit/{Id:guid}`) +- Must reset all private state fields at the TOP of `OnParametersSetAsync` before any awaits +- Must use `try/finally` to ensure `_isLoading` is always cleared +- The loading guard in markup must use a dedicated `_isLoading` boolean, NOT `_model is null && _error is null` + +#### Rationale + +Blazor reuses the same component instance when navigating between different IDs on the same parameterized route. Without a state reset: + +1. Previous post's data remains visible while the next post loads (stale content) +2. Previous error messages persist into a clean new load +3. If `NavigateTo()` is called (post not found) without a `finally` block, `_isLoading` stays `true` through the remaining render cycle, causing a visible stuck loading indicator + +#### Scope + +Applies to: **Legolas** (Frontend / Blazor) for all future parameterized page components. +Applies to: **Gimli** (Tester) — new parameterized page components must have bUnit regression tests covering the parameter-change scenario. + +#### Reference Implementation + +`src/Web/Features/BlogPosts/Edit/Edit.razor` (origin/dev HEAD, commit `dc34ce9`) is the canonical example. + +--- + +### 33. Blazor OnParametersSetAsync must reset all cached state fields + +**Date:** 2026-07-12 +**Author:** Aragorn (Lead) +**PR:** #310 — fix(ui): refresh Edit page loading state on post-ID changes +**Status:** Applied — Tests pass, documented in PR #310 + +#### Context + +PR #310 introduced a `_isLoading` flag managed via `try/finally` to fix indefinite loading states in the Edit page. Copilot flagged, and Aragorn confirmed, that `_model`, `_error`, and `_concurrencyError` were not reset when route parameters change (i.e., `OnParametersSetAsync` re-runs with a new `Id`). This creates a stale UI regression: the previous post's form can render alongside a new error banner. + +#### Decision + +**In any Blazor component that loads data in `OnParametersSetAsync` and caches results in fields, ALL state fields derived from the previous load MUST be cleared at the top of the method before the async operation begins.** + +Standard reset pattern: + +```csharp +protected override async Task OnParametersSetAsync() +{ + _isLoading = true; + _model = null; + _error = null; + _concurrencyError = false; + // ... (other cached state) + try + { + // load data + } + finally + { + _isLoading = false; + } +} +``` + +#### Rationale + +In Blazor Server with `InteractiveServer` rendering, `NavigateTo` inside a component lifecycle method does NOT unmount the component (especially in bUnit). Any state from the previous render cycle persists. The `try/finally` pattern guards only the loading indicator; it does not protect other UI state fields. Failing to reset them causes stale content to bleed through on parameter changes. + +#### Scope + +This rule applies to all components in `src/Web/Features/` that load data in `OnParametersSetAsync`. Code reviews should flag any `OnParametersSetAsync` that omits this reset as a blocking defect. + +#### Review Note + +This decision was surfaced by Copilot's automated inline review on PR #310. Copilot's bug-detection comments should always be treated as first-class blocking signals during review. + +--- + +### 34. PR #313 Follow-up — Shared UserIdClaimsHelper + +**Date:** 2026-05-12 +**Author:** Aragorn +**PR:** #313 review +**Status:** Proposed for future work + +#### Decision + +Recommend creating a shared `UserIdClaimsHelper` (in `src/Web/Security/`) to unify `AuthorId` extraction across `Create.razor` and `Edit.razor`. The helper should implement a single fallback chain: + +```text +ClaimTypes.NameIdentifier → "sub" → Identity.Name → (dev-only) auth0|dev-{guid} +``` + +The dev-only GUID generation should be gated behind `IHostEnvironment.IsDevelopment()`. + +#### Rationale + +PR #313 introduced an inconsistency (flagged by Copilot review comments 1 & 3): + +- `Create.razor` fallback chain: `NameIdentifier → sub → Identity.Name → "dev-user"` (then dead code for GUID) +- `Edit.razor` fallback chain: `NameIdentifier → sub → empty` + +In development environments with non-standard auth (no Auth0), this inconsistency means a post created in Create could have an `AuthorId` that Edit can never reproduce, locking the author out of editing their own post. + +#### Routing + +Assign to **Legolas** (Blazor UI owner). Low priority — production environments are unaffected. + +--- + +### 35. Rich Text Editor — Architectural Plan for Issue #314 + +**Date:** 2026-05-23 +**Decided by:** Aragorn (Lead / Architect) +**Status:** ✅ APPROVED — PR #315 merged. RTBlazorfied substitution accepted + +#### Context + +Issue #314 requests a rich text editor on the Create and Edit blog post pages. The original plan referenced [Blazored.TextEditor](https://github.com/Blazored/TextEditor) (Quill JS wrapper). + +#### Decisions + +##### Decision 35.1: Library Confirmed: Blazored.TextEditor v2.x (Quill JS v2.0.3) + +Blazored.TextEditor is confirmed as the correct choice. It targets Blazor Server (`InteractiveServer`), which matches our render mode. It is a NuGet package with CDN-served JS/CSS, requires no proprietary tooling, and is open-source under the MIT license. + +##### Decision 35.2: Content Storage Format: HTML String + +Store rich text content as an HTML string (output of `GetHTML()`). This is the simplest approach — no transformation layer, no new field, and MongoDB's document model handles arbitrary-length strings without schema changes. The `BlogPost.Content` field and `BlogPostDto.Content` field remain `string` with no type change. + +**Rejected alternative:** Storing Quill Delta JSON and converting to HTML at display time — adds complexity with no benefit at this scale. + +##### Decision 35.3: HTML Sanitization Required Before Storage + +Any HTML accepted from the rich editor MUST be sanitized server-side before storage. Quill produces relatively safe HTML, but XSS hygiene is non-negotiable. Use [HtmlSanitizer](https://github.com/mganss/HtmlSanitizer) (`Ganss.Xss` NuGet package) in the Create and Edit command handlers. + +##### Decision 35.4: Display: MarkupString Rendering Gated on Sanitization + +The list view does not currently display Content body. Any future post-detail page MUST render using `@((MarkupString)sanitizedHtml)` — never `@rawContent`. The sanitization in the write path (Decision 35.3) provides the guarantee. + +##### Decision 35.5: Validation: Custom Content-Present Check Required + +`BlazoredTextEditor` does not participate in Blazor's `EditForm` / `DataAnnotationsValidator` binding. Content must be retrieved via `await _editor.GetHTML()` during `HandleSubmit`, then checked manually before building the command. Quill's "empty" sentinel is `