Claude: changes for issue #91 #92

Merged

Grandiras merged 1 commit from claude/issue-91 into main

2026-08-16 01:18:23 +00:00

claude-bot commented

2026-08-16 01:05:56 +00:00

Collaborator

Closes #91.

claude-bot added 1 commit

2026-08-16 01:05:56 +00:00

Performance and memory pass: the board read, the encryptor, the card preview

CI / build (pull_request) Successful in 39s

Details

94ef5e53cc

The board read was the whole story. GetColumnsAsync hung three collections off
Card in one query, and collection includes multiply: on a 120-card board with
two tags, two assignees and five comments a card it came back 2,400 rows wide,
each repeating that card's encrypted title and description — 2.85 MiB of
ciphertext for 120 cards' worth of text. The third collection bought a number:
a card tile shows how many comments there are and never a word of them, so 600
comment bodies were fetched and AES-decrypted to be thrown away.

Count them instead. Card.CommentCount comes from a GroupBy returning two
integers a card, Card.Comments is documented as not populated by the board
read, and CardView reads the count. Tags and assignees stay in the one query
deliberately — four rows a card is cheaper than the two extra round trips
AsSplitQuery spends avoiding it (~18 ms against ~25 ms measured).

  board load, 120 cards:  ~68 ms / 8.0 MiB  ->  ~18 ms / 2.1 MiB

This runs on every board load and again in every connected circuit on every
BoardNotifier event, so one member dragging a card paid for it once per person
looking.

Two smaller ones, both on paths every render touches:

- AesGcmDataEncryptor's string paths no longer route through EncryptBytes /
  DecryptBytes, which forced an array per call that existed only to be copied
  out of again. Encrypting into the envelope and base64-ing into the returned
  string takes a 780-character value from 6.9 KB of garbage to 2.2 KB, and
  5.4 KB to 1.6 KB back — about a quarter of what a board load allocates now.
  The envelope is unchanged and the new tests carry envelopes written by the
  old implementation to prove it. Scratch buffers are pooled and the ones
  holding plaintext are returned cleared. One AesGcm per call stays: reuse
  measured ~25% faster but needs thread-affine caching of a native handle, for
  ~3% of a board load.

- BoardService.ResolveAsync asks "the board, if it is theirs" in one query
  rather than "the board" then "may they see it", which always cost two round
  trips on two connections. Missing vs Denied still needs the second query, it
  is just off the path where the answer is yes.

- CardPreview moves out of CardView.razor (untestable there, as BoardFilter
  was) and stops flattening 16,000 characters to render 200. Output is
  byte-identical: verified against the old implementation over 300,000 random
  descriptions at four lengths, including the two cases that broke a first
  attempt — a line of pure emphasis still leaves its separator, and marker
  removal cascades (_**_ loses ** and then vanishes as __).

Tests: 33 new unit tests, including the first coverage the encryptor has ever
had. The e2e comment test now creates its own card (it mutates one, so it
should have already) and asserts the badge — the one user-visible thing the
query change could break, and nothing covered it.

No schema change: CommentCount is [NotMapped], and EF reports no pending model
changes.

claude-bot referenced this pull request

2026-08-16 01:05:57 +00:00

Do a performance and memory consumption optimization pass #91

claude-bot commented

2026-08-16 01:06:01 +00:00

Author

Collaborator

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/257); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-92/run-257.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes

2026-08-16 01:08:19 +00:00

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

Solid, well-scoped performance PR with unusually thorough test coverage. I traced the three main changes:

CardService.GetColumnsAsync: drops the third Comments collection include and replaces it with a GroupBy/count query, filling the new [NotMapped] Card.CommentCount. Verified no other reader of Card.Comments exists on cards sourced from this method — CardView.razor is the only consumer and both usages (the metadata-row visibility check on the old line 24 and the badge itself) were updated to CommentCount. CommentList.razor loads its own comments via CommentService.GetForCardAsync, independent of this path, so the dialog's discussion tab is unaffected.
BoardService.ResolveAsync: refactored from GetAsync + CanViewAsync (two round trips) into one query filtered by BoardAccess.VisibleTo, falling back to an existence check only to distinguish Missing from Denied. The outcome semantics match the original exactly (id null → default board fallback; found+visible → Found; found+not-visible → Denied; not found → Missing).
AesGcmDataEncryptor: rewritten string paths using ArrayPool and string.Create, avoiding the byte[] round-trip through EncryptBytes/DecryptBytes. Checked the buffer-sizing arithmetic (Base64Length, MaxDecodedLength), the pool-return/clear discipline (plaintext buffers cleared, ciphertext/envelope buffers not, consistent with what's sensitive), and edge cases (empty string short-circuit, tiny/zero-length payloads). AuthenticationTagMismatchException is not a new API introduced by this PR — it was already used in the unchanged DecryptBytes, so no risk there. The new AesGcmDataEncryptorTests pins byte-for-byte compatibility with legacy envelopes plus a full round-trip sweep across every length mod 3, which is exactly the boundary an off-by-one in this rewrite would hit.
CardPreview: extraction of the markdown-flattening logic into a testable static class with an early-exit optimization (stops walking once it has enough text for the 200-char cutoff). Traced the early-stop logic against the non-early-stop path — they can't disagree, since the loop only breaks once length has already exceeded the bound it would eventually cut to. The CardPreviewTests cover the tricky cases (surrogate pairs, marker cascade ordering, pure-emphasis lines) directly.

The e2e test addition in board.spec.js correctly follows the repo's fixture-ownership convention (creates its own card rather than grabbing .card.first()) and exercises exactly the thing the CommentCount change could break: a live badge update via CommentsChanged without a full reload. Changelog entry is present and dated today's date, matching the requirement in CLAUDE.md.

No correctness, security, or omission issues found.

⏱ 2m 7s · 20 turns · tokens: 98.3k in / 11.6k out (+1869.3k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Solid, well-scoped performance PR with unusually thorough test coverage. I traced the three main changes: - **`CardService.GetColumnsAsync`**: drops the third `Comments` collection include and replaces it with a `GroupBy`/count query, filling the new `[NotMapped] Card.CommentCount`. Verified no other reader of `Card.Comments` exists on cards sourced from this method — `CardView.razor` is the only consumer and both usages (the metadata-row visibility check on the old line 24 and the badge itself) were updated to `CommentCount`. `CommentList.razor` loads its own comments via `CommentService.GetForCardAsync`, independent of this path, so the dialog's discussion tab is unaffected. - **`BoardService.ResolveAsync`**: refactored from `GetAsync` + `CanViewAsync` (two round trips) into one query filtered by `BoardAccess.VisibleTo`, falling back to an existence check only to distinguish `Missing` from `Denied`. The outcome semantics match the original exactly (id null → default board fallback; found+visible → Found; found+not-visible → Denied; not found → Missing). - **`AesGcmDataEncryptor`**: rewritten string paths using `ArrayPool` and `string.Create`, avoiding the byte\[\] round-trip through `EncryptBytes`/`DecryptBytes`. Checked the buffer-sizing arithmetic (`Base64Length`, `MaxDecodedLength`), the pool-return/clear discipline (plaintext buffers cleared, ciphertext/envelope buffers not, consistent with what's sensitive), and edge cases (empty string short-circuit, tiny/zero-length payloads). `AuthenticationTagMismatchException` is not a new API introduced by this PR — it was already used in the unchanged `DecryptBytes`, so no risk there. The new `AesGcmDataEncryptorTests` pins byte-for-byte compatibility with legacy envelopes plus a full round-trip sweep across every length mod 3, which is exactly the boundary an off-by-one in this rewrite would hit. - **`CardPreview`**: extraction of the markdown-flattening logic into a testable static class with an early-exit optimization (stops walking once it has enough text for the 200-char cutoff). Traced the early-stop logic against the non-early-stop path — they can't disagree, since the loop only breaks once length has already exceeded the bound it would eventually cut to. The `CardPreviewTests` cover the tricky cases (surrogate pairs, marker cascade ordering, pure-emphasis lines) directly. The e2e test addition in `board.spec.js` correctly follows the repo's fixture-ownership convention (creates its own card rather than grabbing `.card.first()`) and exercises exactly the thing the CommentCount change could break: a live badge update via `CommentsChanged` without a full reload. Changelog entry is present and dated today's date, matching the requirement in CLAUDE.md. No correctness, security, or omission issues found. --- ⏱ 2m 7s · 20 turns · tokens: 98.3k in / 11.6k out (+1869.3k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-92/run-257.md)