From 37f68a28bcabc49a78012ef0590f8c44d789a52b Mon Sep 17 00:00:00 2001 From: Boromir Date: Mon, 11 May 2026 17:24:40 -0700 Subject: [PATCH 1/4] fix(ui): refresh edit page state on route changes - reset loading state at parameter changes so route updates fetch and render correctly - add bUnit coverage for post ID parameter switch regression Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Web/Features/BlogPosts/Edit/Edit.razor | 1 + .../Web.Tests.Bunit/Features/EditAclTests.cs | 36 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/src/Web/Features/BlogPosts/Edit/Edit.razor b/src/Web/Features/BlogPosts/Edit/Edit.razor index e13db35e..eb2f780d 100644 --- a/src/Web/Features/BlogPosts/Edit/Edit.razor +++ b/src/Web/Features/BlogPosts/Edit/Edit.razor @@ -66,6 +66,7 @@ else if (_model is not null) protected override async Task OnParametersSetAsync() { + _isLoading = true; try { var result = await Sender.Send(new GetBlogPostByIdQuery(Id)); diff --git a/tests/Web.Tests.Bunit/Features/EditAclTests.cs b/tests/Web.Tests.Bunit/Features/EditAclTests.cs index ad9021b8..ec5cf427 100644 --- a/tests/Web.Tests.Bunit/Features/EditAclTests.cs +++ b/tests/Web.Tests.Bunit/Features/EditAclTests.cs @@ -166,6 +166,42 @@ public void EditAllowsAdminToEditAnyPost() cut.Markup.Should().Contain("Edit Post"); } + [Fact] + public void EditShowsNewPostContentAfterParameterChange() + { + // Arrange + var sender = Substitute.For(); + var firstPostId = Guid.NewGuid(); + var secondPostId = Guid.NewGuid(); + const string OwnerSub = "auth0|owner-user"; + + var firstPost = new BlogPostDto(firstPostId, "First Post Title", "First Content", OwnerSub, "Owner", string.Empty, [], DateTime.UtcNow, null, false); + var secondPost = new BlogPostDto(secondPostId, "Second Post Title", "Second Content", OwnerSub, "Owner", string.Empty, [], DateTime.UtcNow, null, false); + + sender.Send(Arg.Is(q => q.Id == firstPostId), Arg.Any()) + .Returns(Task.FromResult(Result.Ok(firstPost))); + sender.Send(Arg.Is(q => q.Id == secondPostId), Arg.Any()) + .Returns(Task.FromResult(Result.Ok(secondPost))); + + Services.AddSingleton(sender); + + // Act — first render + var cut = RenderWithUser( + CreatePrincipalWithSub(OwnerSub, ["Author"]), + parameters => parameters.Add(p => p.Id, firstPostId)); + + cut.Markup.Should().Contain("First Post Title"); + cut.Markup.Should().NotContain("Loading..."); + + // Act — change parameters to a different post + cut.Render(parameters => parameters.Add(p => p.Id, secondPostId)); + + // Assert — second post content shown, loading indicator gone, no stale first-post content + cut.Markup.Should().Contain("Second Post Title"); + cut.Markup.Should().NotContain("First Post Title"); + cut.Markup.Should().NotContain("Loading..."); + } + private IRenderedComponent RenderWithUser( ClaimsPrincipal principal, Action>? configure = null) From 68c6e7bcd001f32fac437bc17c8e6dec9c1b318f Mon Sep 17 00:00:00 2001 From: Boromir Date: Mon, 11 May 2026 17:44:21 -0700 Subject: [PATCH 2/4] fix(ui): reset stale state (_model/_error/_concurrencyError) on parameter changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this fix, OnParametersSetAsync only reset _isLoading. If a first load succeeded (populating _model), then a second route-parameter change triggered a fetch that failed or returned null, the old _model data remained visible alongside the new error message. Changes: - Edit.razor: add _model = null, _error = null, _concurrencyError = false at the top of OnParametersSetAsync so all display state is clean before every fetch cycle - EditAclTests: add two regression tests covering the stale-content paths: • EditClearsStaleContentOnErrorAfterSuccessfulLoad • EditClearsStaleContentOnNullAfterSuccessfulLoad All 92 bUnit tests pass (was 90 before). Gate 4 fully green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Web/Features/BlogPosts/Edit/Edit.razor | 3 + .../Web.Tests.Bunit/Features/EditAclTests.cs | 69 +++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/src/Web/Features/BlogPosts/Edit/Edit.razor b/src/Web/Features/BlogPosts/Edit/Edit.razor index eb2f780d..a5a6ae5b 100644 --- a/src/Web/Features/BlogPosts/Edit/Edit.razor +++ b/src/Web/Features/BlogPosts/Edit/Edit.razor @@ -66,6 +66,9 @@ else if (_model is not null) protected override async Task OnParametersSetAsync() { + _model = null; + _error = null; + _concurrencyError = false; _isLoading = true; try { diff --git a/tests/Web.Tests.Bunit/Features/EditAclTests.cs b/tests/Web.Tests.Bunit/Features/EditAclTests.cs index ec5cf427..6f4761bd 100644 --- a/tests/Web.Tests.Bunit/Features/EditAclTests.cs +++ b/tests/Web.Tests.Bunit/Features/EditAclTests.cs @@ -202,6 +202,75 @@ public void EditShowsNewPostContentAfterParameterChange() cut.Markup.Should().NotContain("Loading..."); } + [Fact] + public void EditClearsStaleContentOnErrorAfterSuccessfulLoad() + { + // Arrange: first load succeeds, second load fails + var sender = Substitute.For(); + var firstPostId = Guid.NewGuid(); + var secondPostId = Guid.NewGuid(); + const string OwnerSub = "auth0|owner-user"; + + var firstPost = new BlogPostDto(firstPostId, "First Post Title", "First Content", OwnerSub, "Owner", string.Empty, [], DateTime.UtcNow, null, false); + + sender.Send(Arg.Is(q => q.Id == firstPostId), Arg.Any()) + .Returns(Task.FromResult(Result.Ok(firstPost))); + sender.Send(Arg.Is(q => q.Id == secondPostId), Arg.Any()) + .Returns(Task.FromResult(Result.Fail("Post could not be loaded."))); + + Services.AddSingleton(sender); + + // Act — first render succeeds + var cut = RenderWithUser( + CreatePrincipalWithSub(OwnerSub, ["Author"]), + parameters => parameters.Add(p => p.Id, firstPostId)); + + cut.Markup.Should().Contain("First Post Title"); + + // Act — second render returns error + cut.Render(parameters => parameters.Add(p => p.Id, secondPostId)); + + // Assert — stale form content gone, error shown + cut.Markup.Should().NotContain("First Post Title"); + cut.Markup.Should().NotContain("Loading..."); + cut.Markup.Should().Contain("Post could not be loaded."); + } + + [Fact] + public void EditClearsStaleContentOnNullAfterSuccessfulLoad() + { + // Arrange: first load succeeds, second returns null (post not found) + var sender = Substitute.For(); + var firstPostId = Guid.NewGuid(); + var secondPostId = Guid.NewGuid(); + const string OwnerSub = "auth0|owner-user"; + + var firstPost = new BlogPostDto(firstPostId, "First Post Title", "First Content", OwnerSub, "Owner", string.Empty, [], DateTime.UtcNow, null, false); + + sender.Send(Arg.Is(q => q.Id == firstPostId), Arg.Any()) + .Returns(Task.FromResult(Result.Ok(firstPost))); + sender.Send(Arg.Is(q => q.Id == secondPostId), Arg.Any()) + .Returns(Task.FromResult(Result.Ok(null))); + + Services.AddSingleton(sender); + var navigation = Services.GetRequiredService(); + + // Act — first render succeeds + var cut = RenderWithUser( + CreatePrincipalWithSub(OwnerSub, ["Author"]), + parameters => parameters.Add(p => p.Id, firstPostId)); + + cut.Markup.Should().Contain("First Post Title"); + + // Act — second render: post not found → should redirect + cut.Render(parameters => parameters.Add(p => p.Id, secondPostId)); + + // Assert — redirected and no stale form content visible + navigation.Uri.Should().EndWith("/blog"); + cut.Markup.Should().NotContain("First Post Title"); + cut.Markup.Should().NotContain("Loading..."); + } + private IRenderedComponent RenderWithUser( ClaimsPrincipal principal, Action>? configure = null) From 1c9ebfaca335896b53cfe85956775cf0f0af186f Mon Sep 17 00:00:00 2001 From: Boromir Date: Mon, 11 May 2026 17:46:23 -0700 Subject: [PATCH 3/4] docs(squad): append Legolas history for PR #310 stale-state fix Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .squad/agents/legolas/history.md | 51 ++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/.squad/agents/legolas/history.md b/.squad/agents/legolas/history.md index 8aedd847..a954c590 100644 --- a/.squad/agents/legolas/history.md +++ b/.squad/agents/legolas/history.md @@ -881,3 +881,54 @@ Then each variant only declares its colour-specific overrides. This is idiomatic - `NavigationManager.NavigateTo` in bUnit changes `.Uri` but does NOT unmount the component — so loading states must be explicitly cleared, not relied on navigation to resolve them **Filed:** `.squad/decisions/inbox/legolas-issue307-ui.md` + +--- + +## 2026-05-11 — PR #310 Unblock: Stale-State Fix on Route Parameter Changes + +### Task + +Resolve reviewer CHANGES_REQUESTED blockers on PR #310 (branch `squad/307-fix-edit-null-post-redirect`): + +1. Merge conflict with dev (caused by prior rebase/squash of PR #309) +2. Stale-state bug: `_model`, `_error`, `_concurrencyError` not reset before each fetch cycle +3. Missing regression tests for the stale-content paths + +### What Was Done + +**Merge conflict resolution:** rebased branch onto `origin/dev` using `git rebase`. Commits `8c7b15f` and `8076fc6` were already upstream (merged as `5e176dc` via PR #309); git skipped them cleanly. Only the new commit `e18b851` was replayed as `37f68a2`. + +**Stale-state fix in `Edit.razor`:** added three reset lines at the top of `OnParametersSetAsync` before `_isLoading = true`: + +```csharp +_model = null; +_error = null; +_concurrencyError = false; +``` + +This guarantees all display state is clean before every fetch cycle, not just `_isLoading`. + +**New bUnit regression tests (EditAclTests.cs):** + +- `EditClearsStaleContentOnErrorAfterSuccessfulLoad` — first fetch succeeds (post A loaded), second fetch fails with error → old form content gone, error shown +- `EditClearsStaleContentOnNullAfterSuccessfulLoad` — first fetch succeeds, second returns null (not found) → redirect to /blog, no stale form content + +### Test Counts + +- Before: 90 bUnit tests +- After: 92 bUnit tests (all pass) +- Full Gate 4 suite: Architecture (16), Domain (42), Web (154), bUnit (92) — all green + +### Learnings + +**Stale-state pattern in Blazor route pages:** + +- Resetting ONLY `_isLoading` is insufficient — ALL display state (`_model`, `_error`, `_concurrencyError`) must be cleared at the top of `OnParametersSetAsync` before the async operation +- If only `_isLoading` is reset, previous successful load data remains and the UI renders stale content alongside new error messages during route parameter changes + +**Rebase vs merge for de-duplicating squash-merged commits:** + +- When upstream squash-merges commits from a branch, a later rebase will skip those commits automatically (`dropping ... patch contents already upstream`) — no manual conflict resolution needed for the already-merged commits +- Use `git rebase --skip` when the first conflict is a commit already upstream + +**Filed:** `.squad/decisions/inbox/legolas-pr310-unblock.md` From 7a6d125de1328f27a2cf16d207eee3ab67844b14 Mon Sep 17 00:00:00 2001 From: Boromir Date: Mon, 11 May 2026 17:59:43 -0700 Subject: [PATCH 4/4] chore(squad): sync squad history and decisions for sprint 19 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .squad/agents/aragorn/history.md | 93 ++ .squad/agents/boromir/history.md | 25 + .squad/agents/gimli/history.md | 57 + .squad/agents/sam/history.md | 20 + .squad/decisions/decisions.md | 1467 +++++++++++++++++ .../inbox/legolas-pr295-review-fixes.md | 23 - 6 files changed, 1662 insertions(+), 23 deletions(-) delete mode 100644 .squad/decisions/inbox/legolas-pr295-review-fixes.md diff --git a/.squad/agents/aragorn/history.md b/.squad/agents/aragorn/history.md index 4935e59c..690d7225 100644 --- a/.squad/agents/aragorn/history.md +++ b/.squad/agents/aragorn/history.md @@ -1323,3 +1323,96 @@ Posted delivery summary comment on issue #291 covering all 5 shipped items: **GitHub cannot self-approve PRs.** When the merging agent is also the PR author, `gh pr review --approve` fails. The correct path is to merge directly (if CI is green and you have maintainer authority) or request a human reviewer. Document this as an expected limitation in squad workflows — don't treat it as a blocking error. **`AuthenticationStateProvider` is the correct DI entry point for Blazor Server auth checks.** `IHttpContextAccessor` is available in the DI container but operates at the HTTP middleware layer — not reliable inside interactive Blazor component lifecycle methods. `AuthStateProvider.GetAuthenticationStateAsync()` is the canonical Blazor-safe way to access the current user inside a component. + +--- + +## 2026-05-11 — Triage: PRs #306 and #308 — Merge conflict + DevOps + duplicate UI fix + +**Requested by:** Boromir (Ralph work-check cycle) + +### Triage Summary + +**Two squad-labeled PRs opened simultaneously, both authored by Boromir:** + +| PR | Title | Status | Decision | +|----|-------|--------|----------| +| #306 | Merge dev: resolve project board automation option IDs conflict (#305) | ✅ **READY FOR REVIEW** | **Route to Boromir (DevOps)** | +| #308 | fix(ui): redirect to /blog when post not found in Edit page (#307) | ⚠️ **CLOSE AS DUPLICATE** | **Superseded by PR #306** | + +### PR #306 Analysis + +**Branch:** `squad/305-sync-board-option-ids` (correct naming convention) +**CI Status:** ✅ All 21 checks passing (squad CI, linting, tests, codecov) +**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 + +**Triage decision:** ✅ **Route to Boromir** (owns DevOps + merge conflict; UI fix is secondary and pre-tested by CI) + +### PR #308 Analysis + +**Branch:** `squad/307-fix-edit-null-post-redirect` (correct naming convention) +**Base commit:** 7e15cdd (BEFORE the merge conflict resolution in PR #306) +**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 + +**Triage decision:** ⚠️ **Close without merge** (superseded by PR #306; both issues #305 and #307 resolved by PR #306) + +### Actions Taken + +1. ✅ Posted triage comment on PR #306: decision, CI status, routing to Boromir +2. ✅ Posted triage comment on PR #308: duplicate explanation, closure recommendation +3. ✅ Created decision record: `.squad/decisions/inbox/aragorn-triage-pr-306-308.md` + +### 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.** + +**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. + +## 2026-07-12 — PR #310 Review: fix(ui): refresh Edit page loading state on post-ID changes + +**Requested by:** Boromir (Ralph). PR author: mpaulosky (squad/307-fix-edit-null-post-redirect → dev). + +**Verdict:** **CHANGES_REQUESTED** — 3 blocking issues. + +### Gate Check Summary + +| Gate | Result | +|------|--------| +| Closes #307 | ✅ | +| Branch is squad/* | ✅ | +| CI green (test suite) | ❌ — only Dependabot/auto-label checks ran; no build/test CI | +| No conflicts | ❌ — CONFLICTING / DIRTY | +| Copilot review addressed | ❌ — 1 flagged bug not yet fixed | + +### Blocking Issues + +1. **Merge conflict** — `mergeable: CONFLICTING`. Must rebase onto current `dev` before merge. +2. **Test CI did not run** — Full test suite (build + unit + bUnit + architecture) never ran on this branch. No CI green signal. +3. **Copilot-flagged stale state bug** — `OnParametersSetAsync` sets `_isLoading = true` but does NOT reset `_model`, `_error`, or `_concurrencyError`. When navigating between post IDs, if the new load fails with an error, the previous post's form still renders alongside the error banner. Fix: reset all three fields at the top of `OnParametersSetAsync`. + +### What Was Good + +- `try/finally` loading state pattern is correct Blazor idiom. +- `role="status"` accessibility addition is good. +- Two new tests (`EditRedirectsToBlogWhenPostNotFound`, `EditShowsNewPostContentAfterParameterChange`) cover real prior gaps. +- Branch naming, PR template, and issue link are all compliant. + +**Routing:** Legolas owns the UI fix. Must also add a regression test for the stale-model-on-error path. + +### Learnings + +**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. + diff --git a/.squad/agents/boromir/history.md b/.squad/agents/boromir/history.md index 255cf4de..8bb29676 100644 --- a/.squad/agents/boromir/history.md +++ b/.squad/agents/boromir/history.md @@ -1453,3 +1453,28 @@ the board only had Todo (`f75ad846`), In Progress (`47fc9ee4`), Done (`98236657` `singleSelectOptions`. Pass all existing option IDs to preserve them; omit `id` for new options. - **Cherry-pick workflow for PR fixes:** stash → checkout origin branch → cherry-pick fix commit → rename to `squad/{issue}-{slug}` → push. Cleaner than diverging 8 commits onto the remote. +### PR #306 Post-Triage Review: Project Board Automation Token/Option ID Sync (2026-05-11) + +**Context:** Aragorn triaged PR #306 with recommendation to route to Boromir for DevOps review. PR bundles three concerns: merge conflict resolution, CI/infra fixes (token + option IDs), and a Blazor redirect fix. + +**Assessment:** + +- ✅ **CI/Infra gate clear:** All 21 checks passing (linting, tests, coverage, CodeQL). Issue #305 linked. No merge conflicts. Branch naming correct. +- ✅ **Workflow logic sound:** Token change (GITHUB_TOKEN → GH_PROJECT_TOKEN) fixes 403 auth errors on project board mutations. Option ID updates (IN_SPRINT, IN_REVIEW, RELEASED) are consistent across 4 workflows. +- ✅ **No regressions:** Documentation added to project-board-audit.yml. markdownlint fix in decisions.md. Secondary UI changes out of scope (routed to Legolas/Gimli). +- ⚠️ **Dependency:** `GH_PROJECT_TOKEN` secret must be configured in repo settings. Already documented in `.squad/decisions/decisions.md`. + +**Routing:** + +- PR author is Boromir (me), so I cannot self-approve per GitHub policy. However, DevOps verification is complete. +- Required parallel reviewers: **Aragorn** (architecture), **Legolas** (Blazor UI), **Gimli** (tests). +- Decision: **READY FOR REVIEWER SPAWN** per playbook. + +**Learnings:** + +- The PR merge process enforces strict separation: PR author cannot approve own changes, even if the changes are infra-only. This is a healthy gate to ensure at least one independent human review. +- Workflow option IDs are a runtime dependency that must be kept in sync across multiple files. A single outdated ID can silently fail board mutations. This PR shows the fix pattern: grep all workflows, update consistently, test in CI. +- The "secondary review" layer (Copilot automated review) is effective at flagging missing tests and logic errors, but does not substitute for domain reviewer verdict. Always route to domain specialist after Copilot. + +**Related Decision:** `.squad/decisions/inbox/boromir-pr306-review.md` (assessment + routing recommendation) + diff --git a/.squad/agents/gimli/history.md b/.squad/agents/gimli/history.md index 270934e0..d02184f2 100644 --- a/.squad/agents/gimli/history.md +++ b/.squad/agents/gimli/history.md @@ -949,3 +949,60 @@ making production changes. and role-management flows, but `PageHeadingComponent` is still validated indirectly through page renders rather than by its own focused component tests. + +## 2026-05-11 — Issue #307 null-post redirect coverage + +### Task + +Verify and validate bUnit test coverage for the missing-post (`Result.Ok(null)`) +path in `Edit.razor`. The bug caused the page to stay on "Loading..." forever +when a post ID was not found. The fix redirects to `/blog` instead. + +### Work Done + +- Confirmed Boromir's fix is already committed on `squad/307-fix-edit-null-post-redirect`: + `Edit.razor` null-value branch now calls `Navigation.NavigateTo("/blog")` instead of + leaving `_model = null`. +- Confirmed `EditRedirectsToBlogWhenPostNotFound` test exists in + `tests/Web.Tests.Bunit/Features/EditAclTests.cs` — asserts `navigation.Uri` + ends with `/blog` when the sender returns `Result.Ok(null)`. +- Validated red/green cycle: test **fails** against unfixed code (stays at + `http://localhost/`), **passes** after fix. +- All 5 `EditAclTests` pass (including auth/redirect coverage for non-owner, + admin, unauthorized submit). +- Full bUnit suite: 88/88 passed after rebuild. + +### Learnings + +1. Always rebuild (`dotnet build`) before running targeted test filters with + `--no-build` — stale binaries can hide real failures or produce false passes. +2. The `NavigationManager.Uri` assertion pattern (`.Should().EndWith("/blog")`) + is reliable for verifying redirects in bUnit without `WaitForAssertion`, + provided the async lifecycle method completes synchronously enough in test + rendering. +3. bUnit correctly handles `NavigateTo` calls from `OnParametersSetAsync` — + the navigation state is observable immediately after `Render()` returns. +4. When another squad member lands a fix before your verification pass, your + role shifts to: confirm the test red/green cycle, verify no existing coverage + was weakened, and document the outcome. + +--- + +## Session: EditShowsNewPostContentAfterParameterChange verification + +**Date:** 2025-07-10 +**Task:** Verify the `EditShowsNewPostContentAfterParameterChange` test is behavior-first and catches the stale UI bug on component parameter reuse. + +### Work Done + +- Read `tests/Web.Tests.Bunit/Features/EditAclTests.cs` and `src/Web/Features/BlogPosts/Edit/Edit.razor`. +- Confirmed the test as written uses `cut.Render(parameters => ...)` (bUnit 2.x API) — the correct method on `IRenderedComponent`. +- The view tool initially displayed a stale version showing `SetParametersAndRender`; the actual file already had `Render`. No production or test code changes were needed. +- All 6 `EditAclTests` pass. Full suite green: Domain 42, Web 154, Web.Tests.Bunit 90, Architecture 16. + +### Learnings + +1. **bUnit 2.x re-render API is `Render`, not `SetParametersAndRender`** — `RenderedComponentRenderExtensions.Render(IRenderedComponent, Action>)` is the correct overload for parameter-driven re-renders. +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. diff --git a/.squad/agents/sam/history.md b/.squad/agents/sam/history.md index 1efc9ba0..936190e0 100644 --- a/.squad/agents/sam/history.md +++ b/.squad/agents/sam/history.md @@ -285,3 +285,23 @@ alternative command dispatchers. - For atomic commits, writing handler tests alongside the implementation is acceptable. Note the crossing in an inbox file so Gimli can audit or extend coverage as needed. + +## Learnings + +### `_isLoading` field initializers only run at construction — not on reuse + +- Blazor Server components are reused across navigation when the router keeps the same component instance (same page, different route params). `OnParametersSetAsync` fires but the constructor does not. +- Any UI state that should reset per parameter change (`_isLoading`, `_error`, `_concurrencyError`) must be explicitly reset at the **top** of `OnParametersSetAsync`, before any `try` block. +- Pattern: `_isLoading = true;` as the first statement ensures every fetch — initial and reuse — shows the loading indicator and hides stale content. + +### bUnit cannot easily assert intermediate async state + +- In bUnit 2.x, `IRenderedComponent` does NOT have `SetParametersAndRender`. The correct method for re-rendering with new parameters is `cut.Render(parameters => ...)` (extension from `RenderedComponentRenderExtensions`). +- `cut.Render(...)` waits for the full async lifecycle to complete before returning. There is no built-in way to pause at `_isLoading = true` and then check markup. +- The practical test strategy for parameter-change behavior: verify the **final state** shows the new data (not stale old data). This proves the lifecycle ran correctly without requiring thread hacks. + +### PR #309 review findings (Aragorn) + +- Always check whether a boolean flag initialized at field level is ever *reset* in `OnParametersSetAsync` before the async work begins. +- File: `src/Web/Features/BlogPosts/Edit/Edit.razor` +- Test: `tests/Web.Tests.Bunit/Features/EditAclTests.cs` → `EditShowsNewPostContentAfterParameterChange` diff --git a/.squad/decisions/decisions.md b/.squad/decisions/decisions.md index 4d7de0fe..db9d21c9 100644 --- a/.squad/decisions/decisions.md +++ b/.squad/decisions/decisions.md @@ -2374,3 +2374,1470 @@ GitHub Actions caches workflow definitions on the `dev` branch. ENV variable upd - **Option A (Fix existing project):** Cannot — old project doesn't exist and can't be recovered - **Option B (Organization project):** Rejected — adds complexity, requires org changes - **Option C (User project + GH_PROJECT_TOKEN) ← CHOSEN:** Simpler, already functional + + +--- + +### 2026-05-11T18:48:17Z: Architecture — PostAuthor value object for #296 + +**By:** Aragorn +**Issue:** #296 + +--- + +## Decision + +Replace the `string Author` field on `BlogPost` with a `PostAuthor` value object. +The `CreateBlogPostCommand` will carry a `PostAuthor` object built in the Blazor component +from the authenticated user's claims — **the handler remains clean and does not touch +`IHttpContextAccessor`** (which is unreliable after the initial HTTP handshake in Blazor Server +interactive SignalR mode). + +Author is **immutable after creation**. The edit flow does not need to touch Author. + +Access-control enforcement ("Authors can only edit their own posts") is **out of scope for +this sprint** and must be a new issue. + +--- + +## PostAuthor Value Object + +```csharp +// src/Domain/ValueObjects/PostAuthor.cs +namespace MyBlog.Domain.ValueObjects; + +public sealed record PostAuthor( + string Id, + string Name, + string Email, + IReadOnlyList Roles); +``` + +- Namespace: `MyBlog.Domain.ValueObjects` (new subdirectory under `src/Domain/`) +- Immutable record — no setters +- `Roles` carries whatever roles `RoleClaimsHelper.GetRoles(user)` returns at creation time +- `Id` = Auth0 `sub` claim; `Name` = `name` claim; `Email` = `email` claim + +--- + +## Domain Change: BlogPost.cs + +**Before:** +```csharp +public string Author { get; private set; } = string.Empty; +public static BlogPost Create(string title, string content, string author) { ... } +``` + +**After:** +```csharp +public PostAuthor Author { get; private set; } = default!; +public static BlogPost Create(string title, string content, PostAuthor author) { ... } +``` + +`BlogPost.Create` guard: +```csharp +ArgumentNullException.ThrowIfNull(author); +ArgumentException.ThrowIfNullOrWhiteSpace(author.Name); +``` + +`BlogPost.Update()` is unchanged — Author is not a parameter and is never mutated after creation. + +**MongoDB storage:** The `PostAuthor` object is stored as an **embedded document** inside the +`blogposts` collection document: + +```json +{ + "_id": "...", + "Title": "...", + "Author": { + "Id": "auth0|abc123", + "Name": "Jane Doe", + "Email": "jane@example.com", + "Roles": ["Author"] + } +} +``` + +--- + +## BlogDbContext Change + +`OnModelCreating` must declare the owned type: + +```csharp +entity.OwnsOne(p => p.Author, a => +{ + a.Property(x => x.Id); + a.Property(x => x.Name); + a.Property(x => x.Email); + a.Property(x => x.Roles); +}); +``` + +MongoDB EF Core provider supports primitive collection properties on owned types. + +--- + +## BlogPostDto Change + +Add flat author fields (avoids a nested DTO for read-only display purposes): + +```csharp +internal sealed record BlogPostDto( + Guid Id, + string Title, + string Content, + string AuthorId, // was: string Author + string AuthorName, + string AuthorEmail, + IReadOnlyList AuthorRoles, + DateTime CreatedAt, + DateTime? UpdatedAt, + bool IsPublished); +``` + +**Rationale for flat fields:** The DTO is consumed by the UI for display only. Flat fields are +simpler to bind in Razor without a nested null-check. The `Author` string property is +**removed** — all call sites must be updated. + +**BlogPostMappings.cs update:** +```csharp +internal static BlogPostDto ToDto(this BlogPost post) => new( + post.Id, post.Title, post.Content, + post.Author.Id, post.Author.Name, post.Author.Email, post.Author.Roles, + post.CreatedAt, post.UpdatedAt, post.IsPublished); +``` + +--- + +## MongoDB Schema: Breaking Change Assessment + +⚠️ **Breaking change.** Existing documents store `Author` as a plain string: +```json +{ "Author": "Test Author" } +``` +Once `BlogPost.Author` becomes a `PostAuthor` owned entity, the EF Core MongoDB provider will +attempt to deserialize the string field as an embedded document and throw. + +**Mitigation for Sprint 19 (dev/test environment):** +1. Drop and recreate the `blogposts` collection in the local dev Atlas deployment. +2. Integration tests (`MongoDbBlogPostRepositoryTests`) already create a fresh database — + no changes needed to test infrastructure. + +**If production data exists:** A one-time migration script must be written before deployment +that reads all documents with a string `Author` field and rewrites them as embedded documents. +This is **out of scope for Sprint 19** — document in the PR and create a follow-up issue. + +--- + +## CreateBlogPostCommand Change + +**Before:** +```csharp +internal sealed record CreateBlogPostCommand(string Title, string Content, string Author) + : IRequest>; +``` + +**After:** +```csharp +internal sealed record CreateBlogPostCommand(string Title, string Content, PostAuthor Author) + : IRequest>; +``` + +The command carries the fully-built `PostAuthor`. The Blazor component (not the handler) is +responsible for reading auth state and constructing `PostAuthor` — keeping the handler +infrastructure-agnostic. + +--- + +## CreateBlogPostCommandValidator Change + +**Remove** the `Author` string validation rules (NotEmpty, MaximumLength). +**Add** a null guard rule for the `PostAuthor` object: + +```csharp +RuleFor(x => x.Author).NotNull().WithMessage("Author is required."); +RuleFor(x => x.Author.Name).NotEmpty().WithMessage("Author name is required.") + .When(x => x.Author is not null); +``` + +--- + +## CreateBlogPostHandler Change + +**No change to constructor or DI.** Handler stays as-is — it calls `BlogPost.Create(title, content, author)` where `author` is now a `PostAuthor` from the command. No `IHttpContextAccessor` needed. + +```csharp +var post = BlogPost.Create(request.Title, request.Content, request.Author); +``` + +The handler is blissfully unaware of where the `PostAuthor` came from. + +--- + +## Create.razor Change + +1. **Remove** the `` input field and its form group. +2. **Remove** `Author` from `PostFormModel`. +3. **Inject** auth state: + ```razor + @inject AuthenticationStateProvider AuthStateProvider + ``` +4. **Build PostAuthor in HandleSubmit:** + ```csharp + var authState = await AuthStateProvider.GetAuthenticationStateAsync(); + var user = authState.User; + var author = new PostAuthor( + user.FindFirst("sub")?.Value ?? string.Empty, + user.FindFirst("name")?.Value ?? string.Empty, + user.FindFirst("email")?.Value ?? string.Empty, + RoleClaimsHelper.GetRoles(user)); + var result = await Sender.Send(new CreateBlogPostCommand(_model.Title, _model.Content, author)); + ``` +5. **Show read-only author name** above the form (optional UX, Legolas to judge): + ```razor +

Author: @_authorName

+ ``` + Populate `_authorName` in `OnInitializedAsync`. + +--- + +## Edit Flow Impact + +- `EditBlogPostCommand` already contains only `(Guid Id, string Title, string Content)` — no Author. +- `BlogPost.Update(title, content)` does not touch Author. +- `Edit.razor` does not display Author. + +**Author is immutable after creation.** No edit-flow changes are needed for this sprint. + +--- + +## Access Control Scope + +The issue description mentions: "only the Admin and Author roles can edit, but Authors can +only edit the Posts they Authored." + +The current `Edit.razor` already has `@attribute [Authorize(Roles = "Author,Admin")]` which +covers the first part. + +**"Authors can only edit their own posts"** (comparing `post.Author.Id` to the current user's +`sub` claim) is **NOT in scope for Sprint 19.** This requires: +- A query-time or UI-time ownership check +- Potentially a 403 response or redirect if a non-owner Author tries to edit + +➡️ **Create a new GitHub issue** for Sprint 19 or 20 titled: +`[Sprint N] feat(app): restrict post editing to post author or Admin` + +--- + +## Work Breakdown + +### Sam (backend) implements: + +| File | Change | +|------|--------| +| `src/Domain/ValueObjects/PostAuthor.cs` | **New file** — PostAuthor record | +| `src/Domain/Entities/BlogPost.cs` | Change `Author` type from `string` to `PostAuthor`; update `Create()` signature and guards | +| `src/Web/Data/BlogDbContext.cs` | Add `OwnsOne` mapping for `PostAuthor` | +| `src/Web/Data/BlogPostDto.cs` | Replace `string Author` with flat `AuthorId`, `AuthorName`, `AuthorEmail`, `AuthorRoles` | +| `src/Web/Data/BlogPostMappings.cs` | Update `ToDto()` mapping | +| `src/Web/Features/BlogPosts/Create/CreateBlogPostCommand.cs` | Replace `string Author` with `PostAuthor Author` | +| `src/Web/Features/BlogPosts/Create/CreateBlogPostCommandValidator.cs` | Replace string rules with null/name guard | +| `src/Web/Features/BlogPosts/Create/CreateBlogPostHandler.cs` | No logic change; compiles via type change | + +### Legolas (UI) implements: + +| File | Change | +|------|--------| +| `src/Web/Features/BlogPosts/Create/Create.razor` | Remove Author input; inject `AuthenticationStateProvider`; build `PostAuthor` from claims in `HandleSubmit`; show read-only author name display | +| `src/Web/Features/BlogPosts/List/Index.razor` | Update any `dto.Author` string references to `dto.AuthorName` | +| `src/Web/Features/BlogPosts/Edit/Edit.razor` | Update any `dto.Author` string references (if displayed) | + +### Gimli (tests) implements: + +| File | Change | +|------|--------| +| `tests/Domain.Tests/Entities/BlogPostTests.cs` | Update all `BlogPost.Create()` calls to pass a `PostAuthor` instead of string | +| `tests/Web.Tests/Handlers/CreateBlogPostHandlerTests.cs` | Update `CreateBlogPostCommand` construction to use a `PostAuthor` | +| `tests/Web.Tests/Features/BlogPosts/Commands/CreateBlogPostCommandValidatorTests.cs` | Rewrite Author validation tests for new rule | +| `tests/Web.Tests/Features/BlogPosts/Commands/CreateBlogPostDomainCommandValidatorTests.cs` | Same — update Author field | +| `tests/Web.Tests/Data/BlogPostMappingsTests.cs` | Update DTO field assertions (`AuthorName` etc.) | +| `tests/Web.Tests.Integration/BlogPosts/MongoDbBlogPostRepositoryTests.cs` | Update any `BlogPost.Create()` calls with `PostAuthor` | + +--- + +## Branch + +`squad/296-post-author-value-object` + + +--- + +# Decision: Test files must be updated in the same PR that changes a constructor signature + +**Date:** 2026-05-15 +**Author:** Aragorn +**PR:** #297 — feat(app): add L1+L2 caching to UserManagement (Auth0 API) +**Status:** Resolved (build repaired) + +## Context + +PR #297 added `IUserManagementCacheService` as a required constructor parameter to `UserManagementHandler`. The production code and infrastructure were complete, but `tests/Web.Tests/Handlers/UserManagementHandlerTests.cs` was not updated. All six construction sites in the test file used the old two-argument signature, causing CS7036 compiler errors that failed CI on both Squad CI and Test Suite. + +## Decision + +**Any PR that changes a public/internal constructor signature must include corresponding test-file updates in the same commit.** CI must not be the first place construction-mismatch errors are discovered. + +## Enforcement + +- Build repair follow-up commit accepted this time (fix: 6d93c77). +- Going forward: Aragorn's PR review gate will explicitly check that all `new Foo(...)` call sites in test projects are consistent with the updated constructor before approving. +- Gimli: when writing or updating handlers, verify test construction sites compile before submitting the PR. + +## Pass-through mock pattern (reference) + +For cache services that wrap an async factory: + +```csharp +private static IUserManagementCacheService BuildPassThroughCache() +{ + var cache = Substitute.For(); + cache.GetOrFetchUsersAsync( + Arg.Any>>>(), + Arg.Any()) + .Returns(ci => new ValueTask>( + ci.Arg>>>()())); + cache.GetOrFetchRolesAsync( + Arg.Any>>>(), + Arg.Any()) + .Returns(ci => new ValueTask>( + ci.Arg>>>()())); + return cache; +} +``` + +A no-op stub returning `default(ValueTask)` would cause tests to receive `null` instead of propagating config-missing exceptions — making config-validation tests silently false-pass. + + +--- + +# Decision: PR #302 cannot merge on a UI-only ownership check + +**Date:** 2026-05-11 +**Author:** Aragorn +**PR:** #302 — feat(ui): restrict blog post editing to post author or Admin (#300) +**Status:** Rejected at review gate + +## Context + +Issue #300 follows #296, which introduced `PostAuthor` with a durable Auth0 `sub` in +`BlogPost.Author.Id`. PR #302 attempted to enforce "only the post author or Admin can edit" by adding +an ownership check in `src/Web/Features/BlogPosts/Edit/Edit.razor` and bUnit tests for the UI +behavior. + +At PR head commit `1dfd970`, the actual write contract remained: + +```csharp +internal sealed record EditBlogPostCommand(Guid Id, string Title, string Content) : IRequest; +``` + +and the handler still updated the post without checking caller identity: + +```csharp +post.Update(request.Title, request.Content); +await repo.UpdateAsync(post, cancellationToken); +``` + +## Decision + +**A Blazor UI check alone is insufficient for edit authorization in MyBlog.** Ownership/admin +authorization must be enforced at the server-side write boundary: + +- `EditBlogPostCommand` must carry caller identity (`CallerUserId`, `CallerIsAdmin`) +- `EditBlogPostHandler` must reject non-admin callers whose `post.Author.Id` does not match + `CallerUserId` +- The UI may still pre-check and redirect for user experience, but it is a convenience layer, not the + security boundary + +## Enforcement + +- PR #302 was rejected at the lead gate +- Legolas and Sam are locked out of the next revision cycle for this artifact per reviewer protocol +- **Gandalf** is the revision owner for the fix cycle because this is an authorization-boundary defect +- Re-review requires: + - handler-level authorization enforcement + - tests that prove unauthorized writes are rejected + - explicit user feedback on denial (`403` or redirect with an error) + +## Consequences + +- Future edit/delete ACL work must treat Razor-page checks as UX only +- Authorization rules for writes belong in handlers or a shared authorization pipeline +- Acceptance criteria that mention "redirect with an error or show 403" require visible denial + feedback, not a silent navigation + + +--- + +# Triage Decision: PRs #306 and #308 + +**Owner:** Aragorn (Lead Developer) +**Date:** 2026-05-11 +**Status:** DECISION + +## Summary + +Triaged two squad-labeled PRs: +- **PR #306**: "Merge dev: resolve project board automation option IDs conflict (#305)" → **ROUTE TO BOROMIR** (ready for review) +- **PR #308**: "fix(ui): redirect to /blog when post not found in Edit page (#307)" → **CLOSE AS DUPLICATE** (superseded by PR #306) + +## Detailed Analysis + +### PR #306 — Status: ✅ READY FOR REVIEW + +**Branch:** `squad/305-sync-board-option-ids` +**Author:** Boromir (DevOps) + +**CI Status:** +- ✅ All 21 checks passing (squad CI, linting, tests, coverage) +- ✅ Codecov gate passing (patch + project) +- ✅ No merge conflicts + +**Content:** This PR resolves THREE distinct concerns bundled together: + +1. **Merge Conflict Resolution** (primary) + - Resolved local `dev` vs `origin/dev` conflict (commit 7e15cdd) + - Correctly retained project board automation option IDs from upstream (commits 135f9fd, 3fb7406) + - Merge commit properly documented in PR body + +2. **DevOps/CI Fix** (infrastructure) + - Switches from `GITHUB_TOKEN` to `GH_PROJECT_TOKEN` in `.github/workflows/add-issues-to-project.yml` + - Updates stale project board option IDs (IN_SPRINT, IN_REVIEW, RELEASED) across workflows + - Adds documentation comments to `project-board-audit.yml` + - Updates `.squad/decisions/decisions.md` (markdownlint fix) + +3. **UX Bug Fix** (included from upstream PR #304 resolution) + - `src/Web/Features/BlogPosts/Edit/Edit.razor`: Redirects to `/blog` when `GetBlogPostByIdQuery` returns `Result.Ok(null)` + - `tests/Web.Tests.Bunit/Features/EditAclTests.cs`: Adds regression test `EditRedirectsToBlogWhenPostNotFound` + - This fix was identified in Copilot's review of PR #304 but missed before merge + +**Copilot Review:** 3 comments generated (reviewed all 7 files) +- No flagged bugs or security issues +- No test or coverage concerns +- Minor documentation/formatting issues (all addressed in diff) + +**Triage Decision:** ✅ **ROUTE TO BOROMIR** +- Boromir owns the DevOps/CI changes and merge conflict resolution +- Boromir should review the workflow logic and option ID corrections +- **Secondary review required:** Aragorn (architecture/patterns) + Legolas (Blazor component changes) +- **Testing:** Already covered by CI (all tests passing, coverage passing) + +--- + +### PR #308 — Status: ⚠️ SUPERSEDED / DUPLICATE + +**Branch:** `squad/307-fix-edit-null-post-redirect` +**Author:** Boromir (DevOps) +**Base commit:** 7e15cdd (BEFORE the merge conflict resolution in PR #306) + +**Content:** Contains ONLY: +- Edit.razor redirect fix (identical to PR #306 commit 8c7b15f) +- EditAclTests.cs regression test (identical to PR #306) + +**Problem:** This PR duplicates the UI changes already in PR #306: +- Commit hashes differ (8c7b15f vs 8cf7981) but content is identical +- PR #308 is based on old commit 7e15cdd (before merge conflict) +- If merged separately, creates merge conflicts in the redirect logic + +**Copilot Review:** 1 comment generated (reviewed 2 files) +- No issues flagged + +**Triage Decision:** ⚠️ **CLOSE WITHOUT MERGE** +- Reason: Duplicate of PR #306 UI changes +- The bug fix WILL be delivered via PR #306 (which covers issue #307) +- Both GitHub issues (#305, #307) will be resolved by PR #306 merge +- Prevents merge conflicts and keeps git history clean + +--- + +## Routing Actions + +1. **PR #306** + - Remove label: `squad` (generic inbox label) + - Add label: `squad:boromir` (Boromir owns DevOps changes) + - Comment posted: Triage decision + review handoff + - Next step: Boromir reviews code; Aragorn + Legolas conduct parallel domain reviews + +2. **PR #308** + - Add label: `duplicate` (GitHub label to mark as duplicate) + - Comment posted: Triage decision + closure recommendation + - Next step: Boromir closes PR with ref to PR #306 + +--- + +## Decision Rationale + +**Why route #306 to Boromir?** +- Primary concern is DevOps/CI (token, option IDs, workflows) → Boromir's domain +- Merge conflict resolution is DevOps responsibility +- UI bug fix is secondary; already tested by Copilot + CI + +**Why close #308?** +- Duplicate content creates merge conflicts if both merge +- Git history is cleaner with single PR per issue +- Both issues are resolved by PR #306 merge +- Prevents reviewer confusion and wasted review time + +--- + +## Related Documents + +- `.squad/team.md` (Boromir = DevOps/Infra) +- `.squad/routing.md` (Boromir reviews CI/infra changes) +- `.squad/playbooks/pr-merge-process.md` (parallel reviewer routing) +- Issue #305 (project board token/option ID sync) +- Issue #307 (Edit page null post redirect) + + + +--- + +# Triage Routing: Issues #298 & #299 + +**Date:** 2026-05-11 +**Decision Maker:** Aragorn (Lead Developer) +**Status:** ✅ Implemented + +## Issue #298: PostAuthor Value Object PR + +**Current State:** +- PR (not issue) labeled only with `squad` (untriaged) +- Title: "feat(domain): introduce PostAuthor value object for blog post authorship (#296)" +- PR body explicitly states: "Working as Sam (Backend Developer)" + +**Decision:** +Route to `squad:sam` — removes `squad` label (triage inbox indicator). + +**Rationale:** +- PostAuthor is a domain value object (Domain layer) +- Changes span: Domain, Infrastructure (MongoDB mapping), DTO, Command/Validation, Tests +- All identified changes are backend/infrastructure concerns +- PR author correctly self-identified as Sam's work +- Legolas noted as downstream (Create.razor UI placeholder needs AuthenticationStateProvider injection) + +**Action Taken:** +- ✅ Added `squad:sam` label +- ✅ Removed `squad` label +- ✅ Posted triage comment noting downstream Legolas work + +--- + +## Issue #299: Pre-Push Gate & Process Alignment + +**Current State:** +- Issue labeled: `squad`, `enhancement`, `squad:legolas`, `go:needs-research` +- Title: "[Sprint 19] fix(process): align worktree pre-push gate with required tests" +- Work: pre-push hook alignment, AppHost.Tests inclusion, process docs reconciliation + +**Decision:** +Reroute from `squad:legolas` → `squad:boromir` — removes `squad` label (triage inbox indicator). + +**Rationale:** +- This is DevOps / CI/CD infrastructure work, NOT frontend +- Per routing.md: CI/CD, build pipeline → **Boromir (DevOps Engineer)** +- Pre-push hook updates require build/test system knowledge +- AppHost.Tests is Aspire infrastructure (Boromir's domain) +- Process docs alignment is Boromir's responsibility +- Legolas owns Blazor UI and components, not worktree enforcement + +**Root Cause:** +Issue was initially misrouted to Legolas (likely boilerplate label applied without domain verification). + +**Action Taken:** +- ✅ Removed `squad:legolas` label +- ✅ Added `squad:boromir` label +- ✅ Removed `squad` label +- ✅ Posted triage comment with routing correction + +--- + +## Team Impact + +Both issues now have correct domain ownership: +- **#298 (Backend):** Sam can proceed with PostAuthor implementation; Legolas queued for UI follow-up +- **#299 (DevOps):** Boromir owns pre-push gate hardening; Sprint 19 blocker now has clear owner + +No architectural decisions or breaking changes introduced — routing only. + + +--- + +# Decision: Add Released Status Option to MyBlog Project Board + +**Owner:** Boromir (DevOps / Infra) +**Date:** 2026-05-11 +**Status:** IMPLEMENTED + +## Problem + +PR #306 (`squad/305-sync-board-option-ids`) contained a silent bug: `RELEASED_OPTION_ID` was set to +`98236657` — the same ID as `DONE_OPTION_ID`. The "Released" status option did not exist on the +MyBlog project board at all. Any workflow attempting to mark items "Released" would silently move them +to "Done" instead, or fail with a GraphQL error on an unknown option ID. + +## Root Cause + +The board's Status field had three options: **Todo** (`f75ad846`), **In Progress** (`47fc9ee4`), +**Done** (`98236657`). There was no "Released" option. When PR #306 was drafted, `RELEASED_OPTION_ID` +was set to match `DONE_OPTION_ID` rather than an actual distinct board option. + +## Action Taken + +1. Queried the project board via `gh api graphql` with keyring auth (unset `GH_TOKEN` to use the + keyring token which has `project` + `read:org` scopes, bypassing the environment `GH_TOKEN` that + lacked `read:org`). +2. Confirmed the "Released" option was absent. +3. Added "Released" (color: BLUE) via `updateProjectV2Field` mutation. New option ID: **`90af7f3b`**. +4. Updated `RELEASED_OPTION_ID` in both workflow files on `squad/305-sync-board-option-ids` (PR #306): + - `.github/workflows/project-board-automation.yml` + - `.github/workflows/squad-mark-released.yml` +5. `DONE_OPTION_ID` (`98236657`) left unchanged. +6. Committed as `fix(ci): set RELEASED_OPTION_ID to 90af7f3b — new distinct board option` and pushed + after all 6 pre-push gates passed (49 tests, 0 failures). + +## Board State After Fix + +| Option | ID | Color | +|------------|------------|--------| +| Todo | `f75ad846` | GRAY | +| In Progress| `47fc9ee4` | YELLOW | +| Done | `98236657` | GREEN | +| Released | `90af7f3b` | BLUE | + +## Key Learnings + +- **Always verify board options exist before coding IDs** — The workflow IDs must reference real board + option IDs. Any mismatch causes silent no-ops or GraphQL errors at runtime. +- **GH_TOKEN env var overrides keyring** — For project board GraphQL, unset `GH_TOKEN` to use the + keyring token which has the required `project` + `read:org` scopes. +- **`updateProjectV2Field` does not take `projectId`** — Only `fieldId` + `singleSelectOptions` are + accepted. Existing options must be passed with their IDs to be preserved. + +## Related + +- PR #306 (`squad/305-sync-board-option-ids`) +- Issue #305 (project board token/option ID sync) +- Workflows: `project-board-automation.yml`, `squad-mark-released.yml` + + +--- + +# PR #306 Post-Triage Assessment: Ready for Review + +**Owner:** Boromir (DevOps / Infra) +**Date:** 2026-05-11 +**Status:** RECOMMENDATION → READY FOR REVIEWER SPAWN + +## Executive Summary + +PR #306 ("Merge dev: resolve project board automation option IDs conflict") has passed all pre-review gates and DevOps infrastructure checks. The PR is **READY FOR PARALLEL REVIEWER SPAWN** (Aragorn + Legolas required per playbook). + +--- + +## DevOps Assessment + +### ✅ CI/Infra Verification Complete + +| Gate | Status | Details | +|------|--------|---------| +| Issue Link | ✅ PASS | `Closes #305` present in PR body | +| CI Status | ✅ PASS | All 21 checks passing (linting, tests, coverage, CodeQL) | +| Coverage Gate | ✅ PASS | codecov/patch + codecov/project both green | +| Merge Conflicts | ✅ PASS | `mergeable: MERGEABLE` | +| Branch Format | ✅ PASS | `squad/305-sync-board-option-ids` follows convention | +| Tests Present | ✅ PASS | EditAclTests.cs includes regression test for post-not-found redirect | + +### ✅ Workflow Logic Review + +**Files Changed (DevOps scope):** +- `.github/workflows/add-issues-to-project.yml` — Token switch: `GITHUB_TOKEN` → `GH_PROJECT_TOKEN` +- `.github/workflows/project-board-automation.yml` — Option IDs updated: IN_SPRINT (61e4505c → f75ad846), IN_REVIEW (df73e18b → 47fc9ee4), RELEASED (8e246b27 → 98236657) +- `.github/workflows/squad-mark-released.yml` — RELEASED_OPTION_ID corrected to 98236657 +- `.github/workflows/project-board-audit.yml` — Documentation comments added (project ID PVT_kwHOA5k0b84BXZpa) + +**Verdict:** +- ✅ Token change is appropriate and necessary for project board mutations (fixes 403 auth errors) +- ✅ Option ID updates are consistent across workflows (no partial updates) +- ✅ Documentation links to recent board automation repair (commits 135f9fd, 3fb7406) +- ✅ No security or CI/CD logic regressions + +### ⚠️ Dependency Alert + +The `GH_PROJECT_TOKEN` secret **must be configured** in repository settings before merge. This is already documented in `.squad/decisions/decisions.md` (lines 2341-2362) and includes setup instructions (Settings → Secrets and variables → Actions). + +### ✅ Secondary Content OK + +While this PR includes non-DevOps changes (Blazor redirect fix + test), they are: +- Already tested by CI (Web.Tests.Bunit passes) +- Reviewed by Copilot (no issues flagged) +- Out of scope for my review (routed to Legolas + Gimli) + +--- + +## Routing Recommendation + +**✅ APPROVE FOR REVIEWER SPAWN** + +**Required Reviewers (per playbook):** +1. **Aragorn** (Lead Developer) — Architecture, patterns, design +2. **Boromir** (DevOps) — *(I've already verified CI/infra; PR author cannot self-approve)* +3. **Legolas** (Frontend/UI) — Blazor Edit.razor redirect logic +4. **Gimli** (Test Specialist) — bUnit test coverage validation + +**Action:** Ralph should spawn Aragorn + Legolas + Gimli for parallel domain reviews. CI/Infra gate is clear. + +--- + +## Anti-Patterns / Blockers + +✅ **None identified.** + +--- + +## Related Documents + +- `.squad/decisions/inbox/aragorn-triage-pr-306-308.md` — Triage decision (routing this to Boromir for DevOps review) +- `.squad/playbooks/pr-merge-process.md` — Reviewer spawn workflow +- `.squad/routing.md` — Reviewer mapping +- Issue #305 — Project board token/option ID sync +- Commits 135f9fd, 3fb7406 — Recent board automation fixes + +--- + +## Decision + +**Status:** ✅ **PROCEED TO PARALLEL REVIEWER SPAWN** + +Ralph: Spawn Aragorn (lead, required) + Legolas (UI) + Gimli (tests) for domain reviews. +Once all reviewers approve and CI remains green → proceed with squash merge. + + + +--- + +# Decision: AppHost.Tests Is a Mandatory Gate 5 Test (Issue #299) + +**Date:** 2026-06-01 +**Author:** Boromir (DevOps) +**Status:** Decided + +## Context + +`tests/AppHost.Tests/AppHost.Tests.csproj` runs Playwright E2E tests via Aspire's +`DistributedApplicationTestingBuilder`. It requires Docker (Aspire boots MongoDB internally +via DCP). It was listed in the playbook's Gate 5 block but was **missing** from the actual +`.github/hooks/pre-push` `INTEGRATION_PROJECTS` array — meaning it was never run locally +before push. + +## Decision + +`AppHost.Tests` is **mandatory in Gate 5** of the pre-push hook (Docker-backed integration +tests). Skipping it locally is equivalent to a silent `--no-verify` for Playwright coverage. + +## Consequences + +- `.github/hooks/pre-push`: `INTEGRATION_PROJECTS` now includes both + `Web.Tests.Integration` and `AppHost.Tests`. +- Playbook and install-hooks summary comments updated to match the actual project list. +- Any future test project additions must be reflected in both the hook's array and the + playbook simultaneously. Use this grep to verify alignment: + + ```bash + grep -r "csproj" .github/hooks/pre-push .squad/playbooks/pre-push-process.md scripts/install-hooks.sh + ``` + +## `--no-verify` Policy + +Unchanged: `git push --no-verify` is prohibited without prior written approval from +Ralph + Aragorn documented in a GitHub issue comment (per playbook hard-block). + + +--- + +# Worktree Pre-Push Gate Investigation + +**Status:** 🔴 CRITICAL FINDINGS +**Date:** 2026-05-11 +**Investigator:** Boromir (DevOps) +**Priority:** P0 — Blocks PRs and allows broken code to reach main + +--- + +## Problem Statement + +Team members report that worktrees can still push code that violates process: +- Code fails tests locally +- New test suites are not being run before push +- Formatting issues slip through + +**Hypothesis:** Hooks not installed, misconfigured, or incomplete. + +--- + +## Investigation Summary + +### ✅ What Works + +1. **Worktree Hook Path Configuration** + - Worktrees do NOT have separate `.git/hooks` directories + - Both main repo and worktrees resolve hooks path to: `.git/hooks` (main repo location) + - Shared hooks approach is CORRECT — single source of truth + +2. **Hook Installation** + - Post-checkout hook calls `scripts/install-hooks.sh` correctly + - Pre-push hook is installed, executable, and up-to-date + - Source (`.github/hooks/pre-push`) matches installed hook + +3. **Hook Delivery to Worktrees** + - Worktrees automatically inherit hooks via shared path + - No worktree-specific bootstrap needed + +### 🔴 ROOT CAUSE: Hook Missing Test Projects + +The **live `.github/hooks/pre-push` hook does not run all required test suites**. + +| Category | Count | Status | +|----------|-------|--------| +| Unit tests in hook | 4 | ⚠️ Incomplete | +| Integration tests in hook | 1 | ⚠️ Incomplete | +| **AppHost.Tests (Playwright E2E)** | 0 | ❌ **MISSING** | +| Playbook expects | 6+ unit tests | ❌ **Hook incomplete** | + +#### What the Hook Actually Runs + +**Unit tests (4):** +- `tests/Architecture.Tests` +- `tests/Domain.Tests` +- `tests/Web.Tests` +- `tests/Web.Tests.Bunit` + +**Integration tests (1):** +- `tests/Web.Tests.Integration` + +#### What's Missing + +**AppHost.Tests** — **MANDATORY per skill and playbook** +- Playwright E2E tests +- Runs under Docker via Aspire DistributedApplicationTestingBuilder +- Currently skipped by local gate + +#### Why This Matters + +When developers push: +1. They run the pre-push hook locally +2. Hook runs 5 test suites but NOT AppHost.Tests +3. AppHost.Tests PASS or FAIL on CI (GitHub Actions) +4. If it fails on CI, the PR is broken and wastes review time + +**No other missing projects currently exist in repo** — Playbook references `Persistence.MongoDb.Tests` and `Persistence.AzureStorage.Tests` which don't exist yet. + +--- + +## Evidence + +### Hook Source vs. Playbook Mismatch + +**Live hook runs:** +```bash +TEST_PROJECTS=( + "tests/Architecture.Tests/Architecture.Tests.csproj" + "tests/Domain.Tests/Domain.Tests.csproj" + "tests/Web.Tests/Web.Tests.Bunit/Web.Tests.Bunit.csproj" + "tests/Web.Tests/Web.Tests.csproj" +) + +INTEGRATION_PROJECTS=( + "tests/Web.Tests.Integration/Web.Tests.Integration.csproj" +) +``` + +**Playbook expects (excerpt):** +``` +dotnet test tests/Architecture.Tests/Architecture.Tests.csproj ... +dotnet test tests/Domain.Tests/Domain.Tests.csproj ... +dotnet test tests/Web.Tests.Bunit/Web.Tests.Bunit.csproj ... +dotnet test tests/Persistence.MongoDb.Tests/Persistence.MongoDb.Tests.csproj ... ❌ (doesn't exist) +dotnet test tests/Web.Tests/Web.Tests.csproj ... +dotnet test tests/Persistence.AzureStorage.Tests/Persistence.AzureStorage.Tests.csproj ... ❌ (doesn't exist) +``` + +**Skill requirements:** +``` +⚠️ AppHost.Tests (Playwright E2E) is MANDATORY. +It must be run locally before every push. +Gate 5: Integration + Playwright E2E — AppHost.Tests included +``` + +--- + +## Root Cause Analysis (Ranked by Likelihood) + +### 1. **Hook Incomplete — AppHost.Tests Missing** (HIGH) + +**Evidence:** +- AppHost.Tests exists in repo (`tests/AppHost.Tests/AppHost.Tests.csproj`) +- Hook doesn't include it in INTEGRATION_PROJECTS array +- Skill explicitly requires it before every push +- Last hook update: 7541e68 (5 commits ago) +- AppHost.Tests has been in repo for many commits + +**Impact:** Every push bypasses E2E tests; regressions only caught in CI. + +### 2. **Playbook Documents Non-Existent Tests** (MEDIUM) + +**Evidence:** +- Playbook lists `Persistence.MongoDb.Tests` and `Persistence.AzureStorage.Tests` +- These projects don't exist in repo +- Playbook also lists non-existent integration variants + +**Impact:** Confuses developers; creates false expectations. Not currently blocking since projects don't exist. + +### 3. **Multiple Hook Backup Files Suggest Rapid Iteration** (LOW) + +**Evidence:** +- 50+ `pre-push.bak.*` files in `.git/hooks/` +- Most recent backups from May 10-11, timestamps 05:00-05:02 +- Suggests `install-hooks.sh` is being run repeatedly + +**Impact:** Noise in hooks directory; no functional impact. Cleanup recommended. + +--- + +## Why Worktrees Can Bypass the Gate + +**Direct Answer:** Worktrees inherit the same incomplete pre-push hook from the main repo. Since AppHost.Tests is missing from the hook, developers can: + +```bash +git push # Runs hook, passes all 5 test suites + # BUT: AppHost.Tests not run + # AppHost.Tests fails on CI → PR blocked (wasted time) +``` + +To intentionally bypass: `git push --no-verify` (requires approval per playbook, but not enforced). + +--- + +## Recommendations + +### Immediate (P0) + +**Fix Hook — Add AppHost.Tests to Gate 5** + +The hook must run AppHost.Tests in the integration test phase. Update `.github/hooks/pre-push`: + +```bash +INTEGRATION_PROJECTS=( + "tests/AppHost.Tests/AppHost.Tests.csproj" + "tests/Web.Tests.Integration/Web.Tests.Integration.csproj" +) +``` + +**Rationale:** +- AppHost.Tests is already in the repo +- Skill requires it before every push +- Fixing the hook prevents E2E regressions from reaching CI + +### Short-term (P1) + +**Reconcile Playbook with Reality** + +Remove references to non-existent test projects from `.squad/playbooks/pre-push-process.md`: +- `Persistence.MongoDb.Tests` → don't list until project exists +- `Persistence.AzureStorage.Tests` → don't list until project exists +- Same for integration test variants + +When these projects are created, update the playbook and hook together. + +**Rationale:** Playbook is authoritative documentation for new team members. + +### Polish (P2) + +**Clean Up Hook Backup Files** + +```bash +cd .git/hooks && rm -f pre-push.bak* +``` + +These are noise and not needed once the hook is stable. + +--- + +## Related Documents + +- **Hook source:** `.github/hooks/pre-push` +- **Install script:** `scripts/install-hooks.sh` +- **Playbook:** `.squad/playbooks/pre-push-process.md` +- **Skill:** `.squad/skills/pre-push-test-gate/SKILL.md` +- **Post-checkout hook:** `.github/hooks/post-checkout` + +--- + +## Sign-off + +**Investigation complete.** The pre-push gate enforcement works correctly for worktrees (shared hooks path, proper bootstrap). The issue is that the hook **implementation is incomplete** — it's missing a required test suite (AppHost.Tests) documented in both skill and playbook. + +**No amount of hook installation or worktree configuration will fix this — the hook source must be updated.** + + +--- + +# Decision: EditBlogPostCommand now carries CallerUserId + CallerIsAdmin + +**Date:** 2026-06-11 +**Author:** Sam (Backend) +**Issue:** #300 + +## What changed + +`EditBlogPostCommand` gains two new required parameters: + +```csharp +internal sealed record EditBlogPostCommand( + Guid Id, + string Title, + string Content, + string CallerUserId, // Auth0 sub claim ("sub") + bool CallerIsAdmin) // user.IsInRole("Admin") + : IRequest; +``` + +The handler now enforces server-side: +- If the post's `Author.Id != CallerUserId` AND `CallerIsAdmin == false` → returns `Result.Fail(..., ResultErrorCode.Unauthorized)`. + +`ResultErrorCode.Unauthorized = 5` has been added to the domain enum. + +## For Legolas (Frontend) + +`Edit.razor` already updated to: +1. Store `_callerUserId` / `_callerIsAdmin` from `AuthStateProvider` in `OnParametersSetAsync`. +2. Pass them when sending the command in `HandleSubmit`. +3. Redirect to `/blog` when `result.ErrorCode == ResultErrorCode.Unauthorized`. + +No further UI changes needed for the authorization redirect flow, but you may want to add a more specific "unauthorized" alert/banner instead of a silent redirect — your call. + +## Error code reference + +```csharp +global::MyBlog.Domain.Abstractions.ResultErrorCode.Unauthorized // = 5 +``` + + +--- + +# Board Sync Repair & Automation Fix + +**Date:** 2026-05-11 +**Status:** In Progress +**Component:** GitHub Project Board Automation + +## Issue + +Project board automation workflows were broken due to hardcoded non-existent project IDs. All issue/PR sync was failing silently: +- `.github/workflows/add-issues-to-project.yml` — Issues not added to board +- `.github/workflows/project-board-automation.yml` — PR state changes not synced +- `.github/workflows/project-board-audit.yml` — Audit not executing +- `.github/workflows/squad-mark-released.yml` — Release sync not executing + +## Root Cause + +Three workflows referenced invalid project ID: `PVT_kwHOA5k0b84BVFTy` (does not exist) + +## Resolution + +### Phase 1: Create New Project ✅ +- Created new project: MyBlog Project Board (https://github.com/users/mpaulosky/projects/5) +- **New Project ID:** `PVT_kwHOA5k0b84BXZpa` +- **Status Field ID:** `PVTSSF_lAHOA5k0b84BXZpazhSmuGY` + +### Phase 2: Status Options Setup ⏳ +Default options created: Todo, In Progress, Done + +**Action Required:** The GitHub GraphQL API does not support adding field options via mutations. Status options must be added manually via GitHub UI: +1. Go to https://github.com/users/mpaulosky/projects/5 +2. Click "Status" field settings +3. Add missing options: "Backlog", "In Sprint", "In Review", "Released" +4. Record their IDs from the field settings + +OR: Rename existing options to match workflow expectations and update workflows accordingly. + +### Phase 3: Update Workflows (Pending) +Once field options are finalized, update all four workflows with: +- New PROJECT_ID: `PVT_kwHOA5k0b84BXZpa` +- New STATUS_FIELD_ID: `PVTSSF_lAHOA5k0b84BXZpazhSmuGY` +- Updated option IDs for: BACKLOG, IN_SPRINT, IN_REVIEW, DONE, RELEASED + +### Phase 4: Test & Verify +- Trigger `add-issues-to-project.yml` with sprint-labeled issue +- Verify issue appears on board with Backlog status +- Create PR linked to issue, verify PR status automations work +- Run `project-board-audit.yml` to check for drift + +## Status Update: Workflow Cache Issue + +**CRITICAL:** GitHub Actions is caching workflow definitions on the `dev` branch. Env var updates are not being picked up even after commits/pushes. This blocks workflow testing on `dev`. + +**Workaround Options:** +1. **Option A:** Manually navigate to project board and configure field options via UI (Backlog, In Sprint, In Review, Released), then update workflows with exact option IDs +2. **Option B:** Dispatch workflows from GitHub UI manually to bypass local caching +3. **Option C:** Create a new branch with updated workflows, test there, then merge to `dev` + +**Current Project Configuration:** +- **Project Name:** MyBlog Project Board +- **Project URL:** https://github.com/users/mpaulosky/projects/5 +- **Project ID (GraphQL):** `PVT_kwHOA5k0b84BXZpa` ✅ (correct) +- **Status Field ID:** `PVTSSF_lAHOA5k0b84BXZpazhSmuGY` ✅ (correct) +- **Current Status Options:** Todo, In Progress, Done (need: Backlog, In Sprint, In Review, Released) +- **GH_PROJECT_TOKEN Secret:** ✅ (created and stored) + +**Files Updated (Awaiting Verification):** +- `.github/workflows/add-issues-to-project.yml` — New PROJECT_ID, GH_PROJECT_TOKEN +- `.github/workflows/project-board-automation.yml` — New PROJECT_ID, new option IDs, GH_PROJECT_TOKEN +- `.github/workflows/project-board-audit.yml` — New PROJECT_ID, GH_PROJECT_TOKEN +- `.github/workflows/squad-mark-released.yml` — New PROJECT_ID, new option IDs + +## Decision + +**Approach:** Complete UI configuration, create fresh branch to bypass cache, test workflows end-to-end +**Owner:** Ralph (work monitor — maintains automation) +**Next Action:** User to configure project field options via UI, Ralph to validate on new branch + +## References +- Project board: https://github.com/users/mpaulosky/projects/5 +- Workflows: .github/workflows/add-issues-to-project.yml, project-board-automation.yml, project-board-audit.yml, squad-mark-released.yml +- GitHub Projects API: https://docs.github.com/en/issues/planning-and-tracking-with-projects/automating-your-project/using-the-api-to-manage-projects + + +--- + +### 2026-05-11T12:36:08Z: Ralph cleanup directive + +**By:** mpaulosky (via Copilot) + +**What:** Incorporate orphan branch/worktree cleanup into Ralph's standard work-check cycle. After scanning for work, Ralph should: +1. Check for merged squad branches (local and remote) +2. List active worktrees and verify they map to open issues +3. Remove orphaned worktrees and delete merged branches +4. Report cleanup results before continuing to next work item + +**Why:** Keep repo state clean between work cycles. User ran manual cleanup and wants it automated as part of Ralph's "go" loop. + +**Integration:** Add to Ralph's Step 1 (Scan for work) — run cleanup checks in parallel with issue/PR scans. Report: "🗑️ Cleaned up {count} branches, removed {count} worktrees." + + +--- + +# Decision: Issue #300 UI — Edit ACL is fully implemented + +**Date:** 2026-07-11 +**Author:** Legolas (Frontend) +**Issue:** #300 +**Branch:** squad/300-restrict-blog-post-edit-to-author-or-admin + +## Status + +All UI work for restricting blog post editing to the post author or Admin is **complete and verified**. + +## What is in place + +### `src/Web/Features/BlogPosts/Edit/Edit.razor` +- `@attribute [Authorize(Roles = "Author,Admin")]` — route-level guard (anonymous users cannot reach the page at all) +- In `OnParametersSetAsync`: after loading the post, reads `sub` claim from `AuthenticationStateProvider`; checks `user.IsInRole("Admin")` and `post.AuthorId == _callerUserId`; **redirects to `/blog`** if neither condition is true +- In `HandleSubmit`: passes `_callerUserId` and `_callerIsAdmin` to `EditBlogPostCommand`; if the server returns `ResultErrorCode.Unauthorized`, shows an inline error message ("You don't have permission to edit this post.") without navigating away + +### `tests/Web.Tests.Bunit/Features/EditAclTests.cs` +Four bUnit tests covering all branches: +1. `EditRedirectsToBlogWhenAuthorIsNotPostOwner` — non-owner Author role → redirected to `/blog` +2. `EditAllowsAccessWhenAuthorIsPostOwner` — matching `sub`/`AuthorId` → form rendered +3. `EditAllowsAdminToEditAnyPost` — Admin role → form rendered regardless of `AuthorId` +4. `EditShowsErrorWhenServerReturnsUnauthorized` — server-side Unauthorized → inline error message shown, no navigation + +## Verification + +- All 88 bUnit tests pass +- All 154 Web.Tests unit tests pass +- Build has 0 errors (pre-existing warnings only) + + +--- + +# Decision: Loading State Pattern for Async Blazor Pages + +**Date:** 2025-07-19 +**Author:** Legolas +**Issue:** #307 + +## Decision + +Use a dedicated `_isLoading = true` flag (not a derived `_model is null && _error is null` condition) +for async-loaded Blazor pages. Wrap the `OnParametersSetAsync` body in `try/finally` to guarantee +`_isLoading = false` on every exit path, including early `return` after `NavigateTo`. + +## Rationale + +The derived condition `_model is null && _error is null` fails to clear when: +- `NavigateTo` + `return` exits early (null post, unauthorized author) +- Any future code path that doesn't set either field + +In bUnit, `NavigateTo` does not unmount the component, so the "Loading..." spinner stays +visible forever in tests and in any real scenario where navigation is delayed. + +## Pattern + +```csharp +private bool _isLoading = true; + +protected override async Task OnParametersSetAsync() +{ + try + { + // ... query and set _model or _error or NavigateTo + } + finally + { + _isLoading = false; + } +} +``` + +```razor +@if (_isLoading) +{ +

Loading...

+} +else if (_model is not null) +{ + // form +} +``` + +## Scope + +Apply this pattern to all Blazor pages with async data loading in `OnParametersSetAsync` +or `OnInitializedAsync` that render a conditional loading state. + +## Files Changed + +- `src/Web/Features/BlogPosts/Edit/Edit.razor` +- `tests/Web.Tests.Bunit/Features/EditAclTests.cs` + + +--- + +# PR #295 Review Fix Decisions + +**Agent:** Legolas +**Date:** 2026-05-07 +**Branch:** squad/291-input-css-fine-tuning + +## Decisions Made + +### `.container-card` gains `mx-auto px-4` + +The `.container-card` utility was bare `max-w-7xl` with no centering or padding. Aligned it to match the app's shared layout pattern (consistent with how `nav` wraps `mx-auto max-w-7xl px-4`). Any future page wrapper that adopts `.container-card` will automatically center and pad correctly. + +### `.btn-secondary` is a solid blue, not an outline button + +The comment said "outline style" but the implementation was always solid blue fill. Chose to fix the comment (not the style) to preserve existing UX. The solid blue secondary button is the canonical pattern going forward. + +### `PageHeadingComponent` falls back to `

` on unknown `Level` + +Added a `default` switch arm that renders `

`. Predictable output over silent empty rendering. + +### All four button variants use fixed palettes (no theme tokens) + +All of `.btn-primary` (green), `.btn-secondary` (blue), `.btn-warning` (amber), `.btn-destructive` (red) use fixed Tailwind colour classes. None follow the user's colour-theme switch. The history.md learning entry from Issue #292 incorrectly stated that primary/secondary used `var(--primary-*)` tokens — corrected. + + +--- + +# Decision: UserManagement L1+L2 Caching Design (Issue #293) + +**Author:** Sam (Backend Developer) +**Date:** 2026-xx-xx +**PR:** #297 +**Status:** Implemented + +## Context + +The ManageRoles.razor page triggered two expensive Auth0 Management API calls on every navigation: +1. `GetUsersWithRolesQuery` — lists all users, then fetches roles per user (N+1 pattern) +2. `GetAvailableRolesQuery` — lists all available roles + +## Decision + +Add a `UserManagementCacheService` following the exact `BlogPostCacheService` pattern (L1 = IMemoryCache, L2 = IDistributedCache/Redis), registered as a singleton via `CachingServiceExtensions.AddUserManagementCaching()`. + +### TTL Values + +- **L1 (IMemoryCache):** 30 seconds absolute expiry +- **L2 (IDistributedCache/Redis):** 2 minutes absolute expiry + +These are shorter than blog post TTLs (1min/5min) because user/role data is more sensitive to staleness. + +### Cache Keys + +- `usermgmt:users` — all users with their roles +- `usermgmt:roles` — all available roles (separate key, invalidated independently) + +### Invalidation Strategy + +- `AssignRoleCommand` and `RemoveRoleCommand` handlers call `InvalidateUsersAsync(CancellationToken.None)` after the Auth0 mutation succeeds. +- The `GetAvailableRolesQuery` result (role list) is NOT invalidated on assign/remove — roles themselves don't change, only user-role assignments do. + +## Alternatives Considered + +1. **HTTP client-level caching** — Rejected: more complex, harder to invalidate explicitly. +2. **Invalidating roles on mutation** — Rejected: role definitions don't change when assigning/removing roles from users. +3. **Single cache key for both** — Rejected: separate keys allow independent expiry and selective invalidation. + +## Impact + +- Reduces Auth0 Management API calls from N+2 per page load to 0 during cache hits. +- Under normal usage (multiple page navigations within 2 minutes), API calls drop to once per 2-minute window. +- Cache invalidation ensures role changes are visible within 30 seconds (next L1 miss). + + +--- + +# Decision: Issue #300 — Backend Authorization in EditBlogPostHandler (Audit) + +**Date:** 2026-06-11 +**Author:** Sam (Backend) +**Issue:** #300 + +## Status + +✅ Already fully implemented. This note audits and confirms correctness. + +## Authorization Rule + +Only the post's **author** (`post.Author.Id == CallerUserId`) or a user with the +**Admin role** (`CallerIsAdmin == true`) may edit a blog post. + +## Where the check lives + +The rule is enforced in `EditBlogPostHandler.Handle(EditBlogPostCommand)` — the +smallest correct place because: + +- It runs on every code path that edits a post (Blazor UI, any future API caller). +- It has access to the persisted post (to read `Author.Id`) before mutating it. +- It keeps the invariant out of the Razor component where it could be bypassed. + +```csharp +if (!request.CallerIsAdmin && post.Author.Id != request.CallerUserId) + return Result.Fail("You are not authorized to edit this post.", ResultErrorCode.Unauthorized); +``` + +`CallerUserId` is the raw Auth0 `sub` claim; `CallerIsAdmin` is derived from +`user.IsInRole("Admin")`. Both are supplied by the `Edit.razor` component via +`AuthenticationStateProvider` and passed on the command. + +## Why NOT in FluentValidation + +The validator (`EditBlogPostCommandValidator`) validates data shape (non-empty Id, +Title, Content). Authorization is a behavioural rule that requires the persisted +post's author identity — information not available at validation time. Mixing +authorization into the validator would require a database call inside FluentValidation, +which violates the single-responsibility principle and complicates testability. + +## Test coverage + +All three authorization scenarios are covered in +`tests/Web.Tests/Handlers/EditBlogPostHandlerTests.cs`: + +| Scenario | Test method | Expected | +|---|---|---| +| Author edits own post | `HandleEdit_AuthorCanEditOwnPost_ReturnsSuccess` | `Success == true` | +| Admin edits any post | `HandleEdit_AdminCanEditAnyPost_ReturnsSuccess` | `Success == true` | +| Non-admin non-author | `HandleEdit_DifferentNonAdminUser_ReturnsUnauthorized` | `ErrorCode == Unauthorized` | + +All 154 `Web.Tests` pass as of this audit. + +## Note to Gimli + +The handler tests were written in the same commit as the implementation (`ee0aafb`). +If additional edge-case coverage is desired (e.g., empty `CallerUserId` with +non-admin → Unauthorized), that is Gimli's territory. The current three tests +cover the functional requirements of issue #300. + + +--- + +# Test Ownership Note: Issue #300 Backend Authorization Tests + +**Date:** 2026-06-11 +**Author:** Sam (Backend) +**Issue:** #300 + +## Context + +Per Sam's charter, test files are owned by Gimli. The handler tests for issue +#300 were written in the same commit as the backend implementation (`ee0aafb`) +to keep the change atomic. All three authorization tests are in: + +``` +tests/Web.Tests/Handlers/EditBlogPostHandlerTests.cs +``` + +## Tests already written (in scope) + +| Method | What it covers | +|---|---| +| `HandleEdit_AuthorCanEditOwnPost_ReturnsSuccess` | Author === CallerUserId, non-admin | +| `HandleEdit_AdminCanEditAnyPost_ReturnsSuccess` | CallerIsAdmin = true, any author | +| `HandleEdit_DifferentNonAdminUser_ReturnsUnauthorized` | CallerUserId != author.Id, non-admin → `ResultErrorCode.Unauthorized` | + +## Possible gaps for Gimli to consider + +- Empty `CallerUserId` with `CallerIsAdmin = false` → should return Unauthorized + (the handler's check will fire since `"" != post.Author.Id` for any real author) +- Domain.Tests coverage: `PostAuthor.Id` equality comparison (if not already present) + +## No further action required from Sam on tests + +All required authorization scenarios are validated. Ball is with Gimli if +additional coverage is desired. + + +--- + +# Decision: Edit page `_isLoading` must be reset at the top of `OnParametersSetAsync` + +**Author:** Sam +**Date:** 2026-06-11 +**Relates to:** PR #309 review by Aragorn + +## Decision + +`_isLoading = true;` must be the **first statement** in `OnParametersSetAsync` (before the `try` block), not just a field initializer. + +## Rationale + +Blazor Server components can be reused across navigation events — the router calls `OnParametersSetAsync` with the new `Id` without destroying and re-creating the component. When that happens: + +- The field initializer (`private bool _isLoading = true;`) only runs at construction time. +- Without resetting `_isLoading` at the top of `OnParametersSetAsync`, the flag is `false` from the previous fetch. +- The template renders the stale `_model` form content while the new fetch is in flight, because `_isLoading = false` means the `else if (_model is not null)` branch is entered immediately. + +Setting `_isLoading = true` before the `try` guarantees: +1. The loading indicator shows at the start of every fetch (initial + reuse). +2. The `finally` block always brings it back to `false`, so no state is leaked. + +## Affected Files + +- `src/Web/Features/BlogPosts/Edit/Edit.razor` +- `tests/Web.Tests.Bunit/Features/EditAclTests.cs` + +## Test Coverage + +Added `EditShowsNewPostContentAfterParameterChange` — renders with post A, changes `Id` to post B, asserts post B data and no stale post A content. diff --git a/.squad/decisions/inbox/legolas-pr295-review-fixes.md b/.squad/decisions/inbox/legolas-pr295-review-fixes.md deleted file mode 100644 index ff4f72f7..00000000 --- a/.squad/decisions/inbox/legolas-pr295-review-fixes.md +++ /dev/null @@ -1,23 +0,0 @@ -# PR #295 Review Fix Decisions - -**Agent:** Legolas -**Date:** 2026-05-07 -**Branch:** squad/291-input-css-fine-tuning - -## Decisions Made - -### `.container-card` gains `mx-auto px-4` - -The `.container-card` utility was bare `max-w-7xl` with no centering or padding. Aligned it to match the app's shared layout pattern (consistent with how `nav` wraps `mx-auto max-w-7xl px-4`). Any future page wrapper that adopts `.container-card` will automatically center and pad correctly. - -### `.btn-secondary` is a solid blue, not an outline button - -The comment said "outline style" but the implementation was always solid blue fill. Chose to fix the comment (not the style) to preserve existing UX. The solid blue secondary button is the canonical pattern going forward. - -### `PageHeadingComponent` falls back to `

` on unknown `Level` - -Added a `default` switch arm that renders `

`. Predictable output over silent empty rendering. - -### All four button variants use fixed palettes (no theme tokens) - -All of `.btn-primary` (green), `.btn-secondary` (blue), `.btn-warning` (amber), `.btn-destructive` (red) use fixed Tailwind colour classes. None follow the user's colour-theme switch. The history.md learning entry from Issue #292 incorrectly stated that primary/secondary used `var(--primary-*)` tokens — corrected.