Skip to content

fix(picker): #69 のマージ後レビュー指摘への対応と 0.24.0 への bump - #70

Merged
GeneralD merged 7 commits into
mainfrom
fix/pr69-post-merge-review
Nov 3, 2026
Merged

GeneralD merged 7 commits into
mainfrom
fix/pr69-post-merge-review

Conversation

@GeneralD

@GeneralD GeneralD commented Nov 3, 2026 •

Copy link
Copy Markdown
Owner

agent type follow-up version

Follow-up of #69

概要

PR #69(#12: ピッカーにサムネイルと説明のホバーを出す)は、レビューが走る前に 36d8a43 でマージされ、バージョンも上げられていませんでした(main は 0.23.0 のまま)。マージ後に 3 人が独立してレビューした指摘(重複を除くと 12 件ほど)を、main の実コードに当てて 1 件ずつ検証しました。

  • main の CI(36d8a43)は全部成功でした: Test の run 40992393617 は check / preflight / test-gpu / release-build / lint / test がすべて success です。main は赤くありません。
  • 妥当だった指摘は直しました。測定したら前提が違った指摘(毎回のデコード、NSBitmapImageRep の初期化コスト)は、コードを足さず文書とコメントを実測値に直しました。
  • feat: の feat(picker): ピッカーのセルにマニフェストのサムネイルを描き、ホバーに説明を加える #69 が上げ損ねた minor を 0.24.0 として上げます(chore(release) の 1 コミット)。

変更点

  • 説明のホバー上限が主張どおりでなかった(must-fix、f46bca2): コメントと文書は「manifest がファイルの大きさのツールチップを作れない」と言っていましたが、上限は Character 数で数え(結合文字 1000 個を付けた 1 文字が通る)、切る前に空白を削り(余白で 280 字を超えたり、省略記号の前に隙間が残ったりする)、キー入力のたびに全文を読んでいました。Unicode スカラー数で数え、先頭だけを読み、省略記号は切り口のあとに空白以外が続くときだけ付け、主張を「説明だけを抑える。名前と作者は切らない」に直しました(Presenter のコメント、README、ARCHITECTURE)。
  • artwork(of:) が可用性を見ていなかった(nit、7d8ea69): help(of:) は broken の説明を無視するのに、画像は Repository が渡さないことに頼っていました。Presenter 側でも availability == .ready を見るようにし、テストの broken 行に実際にサムネイルを持たせました。説明だけが変わった listing が新しい state になることを、UseCase(EffectCatalogThumbnailTests)と Presenter(ホバーが変わる)の両方で固定しました。
  • デコーダのテストが足りなかった(should-fix 3 件と nit、ee92e9e): 同梱の Examples/Ripple.lens/thumbnail.png(256 × 256 RGB)を実際にデコードして一色でないことを確認、単色 PNG / JPEG が元の色で返ること、サイズ 0 の PNG が nil であること、解像度(pt)が半分の画像でも画素数で上限を判定すること(2049 × 2049 px・1024.5 pt は nil)を足しました。「途中で切れた」テストは実際に固定している範囲(ヘッダの途中)に名前を直しました。testing.md の読み取り例外に、2 つ目の読み手として EffectThumbnailDecodingTests を明記しました。
  • 決定の文書とコメント(a5e312a): 1.7 MB の PNG を上限の根拠にしていましたが、カタログの 1 MiB 上限で先に弾かれる入力でした。1 MiB 以内の 15000 × 15000 グレー PNG(982,742 byte)で測り直し、開くのに約 1 ミリ秒でメモリ増はなく、描画で約 226 MB・0.34 秒かかる、と書き換えました。「サムネイルは小さい」「描画のたびにデコードする」という Known limits を、数えた結果と実際の最悪ケースに直し、ImageIO を採らないのは「import 一覧が許さない」からではなく選択であると書き直しました。途中で切れた PNG / JPEG は、切れた位置から先が黒(PNG)や灰(JPEG)で描かれ、ImageIO は complete と答えることを観測どおりに記録しました。デコード失敗と上限超過はログに出ないことを README / ARCHITECTURE に明記しました。architecture-boundaries.md の Router 行、.claude/CLAUDE.md のチェーン注記、AGENTS.md(2048 × 2048 の上限)、EffectThumbnail のコメント、Package.swift の Router 行を、実装に合わせました。ARCHITECTURE の「ここで吸収した」という履歴語りは決定の文に直し、Lane 1 の破損 PNG の手順には動かして確かめた printf コマンドを足しました。
  • EffectArtworkResolutionTests.nothingIsRemembered はキャッシュの有無を約束するように読めたので、「リゾルバは何も覚えない」という意味の名前に直しました(ee92e9e)。
  • Copilot の指摘 2 件(レビュー後に追加): 上限の判定 pixelsWide * pixelsHigh は、ヘッダが UInt32.max 近い辺を宣言するとトラップするので、積を使わず width <= maxPixels / height で判定し、0 の辺も同じ guard で弾きます(c5f9689)。切り口のあとの空白を最後まで読んでいたので、見るのを 280 スカラーまでに抑え、その境界をコメント・ARCHITECTURE・テストの 1 ケースに書きました(b39fd97)。
  • version.txt を 0.23.0 から 0.24.0 に上げました(chore(release))。

採用しなかった指摘

  • 毎回のデコードを避けるキャッシュ / ImageIO の縮小デコード(should-fix 2 件): 前提を測って確かめました。オフスクリーンの NSHostingView にデコーダの代わりのカウンタを置き、絵のあるセル 9 つ + タイルのセル 2 つで数えたところ、最初の描画が 9 回、同じ内容の 2 回目が 0 回、検索で表示セルが変わった 1 回が 3 回、さらに絞った検索が 0 回、ウィンドウを広げて 0 回でした。キー入力やリサイズのたびに全セルがデコードされるわけではなく、セルが作り直されたとき(LazyVGrid から出入りしたときや画像が変わったとき)だけです。コストが見えていないものに、キャッシュか第二の経路を足さないことにしました。ARCHITECTURE の Known limits に、数えた結果と、遅くなったら次に取る手として書きました。
  • NSBitmapImageRep(data:) が先にデコードしているから上限が効かない(should-fix): 測って否定されました。上と同じ 982,742 byte のファイルは、開くのに約 1 ミリ秒でメモリ増はなく、コストは描画時にかかります。ただし遅延はドキュメントされた約束ではないため、その旨をコメントに書きました。
  • 上限判定を Presenter に移す(案 a: デコーダが画素数を返す)(should-fix): 上限は表示の方針ではなく、OS のデコード呼び出しを守る資源の上限なので、呼び出しと同じ場所に置いたままにしました。案 b(architecture-boundaries.md などに明記)を採っています。
  • ヘッダだけの巨大な PNG を使った上限テスト(nit): 測ったところ、1 × 1 より大きな宣言に空の IDAT を付けた PNG は 2048 × 2048 でも NSBitmapImageRep が nil を返すので、上限で弾かれたのかデコード失敗なのかを区別できません。実画像の 2049 × 2049 / 4097 × 1024 と pt 違いのテストで上限を固定しています。
  • 途中で切れた PNG / JPEG を nil にする(should-fix): 切れた位置から先が黒や灰で描かれる挙動を確かめました(ImageIO の状態は complete のまま)。末尾の検査は PNG の IEND なら確実ですが、JPEG は終端マーカーのあとに余りがあっても有効で、末尾が欠けても復号できるため、一貫した判定になりません。書きかけのファイルは次の読み直しで置き換わるので、挙動を文書に書くにとどめ、OS 依存の挙動をテストで固定することもしませんでした。
  • 名前・作者の上限と行数の上限(must-fix の一部): 名前と作者はメニューとセルの題名にも全文で出るもので、manifest の読み込みが上限なしなのは DataSource 側の別の話です。行数は 280 スカラーで 280 行が上限になり、パッケージの作者自身の manifest です。そのため上限の主張を「説明だけ」に直しました。
  • ピッカーの描画をコミットするテスト(should-fix): .claude/CLAUDE.md の「SwiftUI の view body は unit test しない」というルールを変える判断になるので入れていません。代わりに、使い捨ての確認として、ピッカーをオフスクリーンで描き(Ripple 6 セル、壊れた PNG、上限超えの PNG、途中で切れた PNG、broken、同梱エフェクト)、画像を目で見て確かめました。これは手元の一回限りの確認で、画像の見た目に自動テストはありません(ARCHITECTURE にも書きました)。
  • feat(picker): ピッカーのセルにマニフェストのサムネイルを描き、ホバーに説明を加える #69 の本文と エフェクト選択 UI: Snap Camera 風サムネイルグリッドピッカー #12 の本文を書き換える(nit): マージ済みの PR の本文は書き換えず、訂正はこの PR と feat(picker): ピッカーのセルにマニフェストのサムネイルを描き、ホバーに説明を加える #69 へのコメントに残します。「外せるのは maxPixels の判定とそのテストだけで、デコーダのファイル全体ではない」という訂正をそこに書きます。

テスト

b39fd97 でローカルのゲートを通しました(gate.py、全段 PASS)。test は 1 回、無関係な Mach loopback の a failed register gives back the receive right it allocated(プロセス内の受信権の増減を数えるテストで、並列に走る他のテストの影響と思われますが、原因は確かめていません)で落ち、make test の再実行とゲートの通し直しはどちらも通っています。

  • format: PASS(差分なし)/ lint: PASS / check: PASS / build: PASS / build-dev-harness: PASS
  • test: PASS — Test run with 1052 tests in 139 suites passed ... with 12 known issues(12 件は main に元からある既知の issue で、このPRの追加分ではありません)
  • test-gpu: PASS — Test run with 27 tests in 6 suites passed
  • 足したテスト: EffectPickerPresenterTests(余白 3 通り、結合文字の洪水、broken がサムネイルを持つ場合、説明の差し替え)、EffectCatalogThumbnailTests(説明だけの差し替え)、EffectThumbnailDecodingTests(同梱 Ripple、単色 PNG / JPEG、サイズ 0、pt 半分の画像、ヘッダの途中の切断)。
  • 測定は一回限りの手元の確認です(コミットしていません)。NSBitmapImageRep の初期化と描画のコスト、デコード回数のカウント、ピッカーのオフスクリーン描画の目視。見た目の自動テストはありません。

…the cap bounds

The cap on the hover's description was documented as keeping a tooltip from being the size of a file, which it did not: it counted Characters (one letter under a thousand combining marks passed), trimmed before cutting (so padding could push a 280-character description over, and a cut could leave a gap before the ellipsis) and re-read the whole text on every keystroke. It now counts Unicode scalars, reads only the start of the text, ends in the ellipsis only when more than whitespace follows the cut, and the comment, README and ARCHITECTURE say that it bounds the description alone.
…ts entry carries

help(of:) already ignored a broken package's description, but artwork(of:) relied on the catalog never handing one a thumbnail, and its test used a fixture that had none. It now gates on availability itself and the test gives the broken entry a thumbnail. A description-only listing is now pinned as a new state at the UseCase and as a new hover at the Presenter, as a picture-only one already was.
… and to pixels-not-points

EffectThumbnailDecodingTests decoded only synthetic 8-bit grey bitmaps and cut a picture inside its header. They now also decode Examples/Ripple.lens/thumbnail.png (256 x 256, RGB) and check that it is not one flat colour, check that a solid colour comes back as that colour in both formats, pin that a zero-size PNG is nil, and judge the 2048 x 2048 limit by pixels with a picture whose points are half its pixels. The truncation test says what it pins (a cut inside the header); a cut inside the pixel data decodes to a partly drawn image on this OS, which is recorded in the suite and in ARCHITECTURE rather than held to. The decoder comment gives the measured cost of an over-limit file.
…here the Router holds a decode

ARCHITECTURE quoted a 1.7 MB PNG as the case the 2048 x 2048 limit guards against, which the catalog's 1 MiB limit would refuse first; it now quotes a 15 000 x 15 000 PNG of 982 742 bytes with its measured open and draw costs. The Known limits paragraph said a thumbnail is small and that the decode repeats on every body pass; it now gives the counted behaviour (once per built cell) and the real worst case, and ImageIO is stated as a choice and not as an import-list rule. A picture cut off inside its pixel data decodes partly and is written down as such, and that an undecodable or over-limit picture is not logged is stated in README and ARCHITECTURE. The Router row of architecture-boundaries.md, the chain note in CLAUDE.md, AGENTS.md, the Entity comment and the Package.swift label now name the picker's thumbnail decode and its pixel limit, and Lane 1 gets the command that makes its corrupt-PNG file.
Copilot AI lite review requested due to automatic review settings November 3, 2026 05:53
@GeneralD GeneralD self-assigned this Nov 3, 2026
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 46 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6c298781-369d-44b4-9713-eb8406f21661
Commits

Reviewing files that changed from the base of the PR and between abbca5e and b39fd97.

Files selected for processing: 4
  • Sources/AppRouter/EffectThumbnailDecoding.swift
  • Sources/Presenters/EffectPickerText.swift
  • Tests/PresentersTests/EffectPickerPresenterTests.swift
  • docs/ARCHITECTURE.md

No actionable comments were generated in the recent review.

Walkthrough

The picker now uses artwork only for ready entries and limits trimmed descriptions to 280 Unicode scalars. Thumbnail decoder tests cover image rendering and rejection cases. Documentation describes the decoder’s pixel limit, fallback behavior, and related picker behavior.

Changes: Effect picker updates

Layer / File(s) Summary
Thumbnail decoding contract and tests
.claude/CLAUDE.md, .claude/rules/*, AGENTS.md, Package.swift, README.md, Sources/AppRouter/EffectThumbnailDecoding.swift, Sources/Entity/EffectThumbnail.swift, Tests/AppRouterTests/EffectThumbnailDecodingTests.swift, docs/ARCHITECTURE.md
Documentation describes the 2048 × 2048 pixel limit, decode failure fallback, partial image rendering, and decoder responsibilities. Decoder tests cover PNG and JPEG color preservation, the shipped Ripple thumbnail, truncated headers, zero dimensions, and pixel dimensions versus point dimensions.
Picker artwork and description display
Sources/Presenters/EffectPickerText.swift, Tests/EffectCatalogUseCaseTests/EffectCatalogThumbnailTests.swift, Tests/PresentersTests/*, README.md, docs/ARCHITECTURE.md, version.txt
Artwork selection now uses thumbnails only for ready entries. Descriptions are trimmed and limited to 280 Unicode scalars, with an ellipsis when additional non-whitespace text follows. Tests cover description display updates and catalog state changes without additional preparation. The version changes to 0.24.0.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related PRs

  • GeneralD/mirage#65: Adds the effect picker and the contracts used by the picker changes in this PR.
  • GeneralD/mirage#59: Introduces .lens manifest descriptions, which this PR trims and displays.
  • GeneralD/mirage#56: Implements catalog state publication, which the description-update test exercises.
Merge Risk: Low · previous review abbca

Search may lag when a package has an unusually long description ending in whitespace. This is a bounded merge risk, not a reason to block the release.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title accurately identifies the picker review follow-up and the version bump to 0.24.0, which are the main changes in the pull request.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 8 files. (8 skipped: 8 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
Generate docstrings
  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9816119e-4337-4529-817c-f4e885ed5560
Commits

Reviewing files that changed from the base of the PR and between 36d8a43 and abbca5e.

Files selected for processing: 16
  • .claude/CLAUDE.md
  • .claude/rules/architecture-boundaries.md
  • .claude/rules/dev-verification.md
  • .claude/rules/testing.md
  • AGENTS.md
  • Package.swift
  • README.md
  • Sources/AppRouter/EffectThumbnailDecoding.swift
  • Sources/Entity/EffectThumbnail.swift
  • Sources/Presenters/EffectPickerText.swift
  • Tests/AppRouterTests/EffectThumbnailDecodingTests.swift
  • Tests/EffectCatalogUseCaseTests/EffectCatalogThumbnailTests.swift
  • Tests/PresentersTests/EffectArtworkResolutionTests.swift
  • Tests/PresentersTests/EffectPickerPresenterTests.swift
  • docs/ARCHITECTURE.md
  • version.txt

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Two active issues remain: an integer-overflow crash is possible when validating untrusted thumbnail dimensions, and description matching can still scan unbounded whitespace-padded suffixes.

2 open issues
What changed in this PR

This follow-up addresses post-merge review findings from #69, improves effect-picker descriptions and thumbnail handling, expands regression coverage, updates documentation, and bumps the version to 0.24.0.

Changes:

  • Truncates descriptions by Unicode scalar count while preserving full names and authors.
  • Restricts thumbnail artwork to ready entries and expands decoder/catalog test coverage.
  • Synchronizes architecture, testing, and verification documentation.
  • Updates version.txt from 0.23.0 to 0.24.0.
  • Open findings remain around overflow-safe pixel-limit handling and unbounded whitespace scanning.

This pull request requires changes.

File Summary
version.txt Bumps the version to 0.24.0.
Tests/​PresentersTests/​EffectPickerPresenterTests.swift Adds coverage for description trimming, scalar limits, updates, and broken-entry artwork.
Tests/​PresentersTests/​EffectArtworkResolutionTests.swift Clarifies the resolver test name and no-memory behavior.
Tests/​EffectCatalogUseCaseTests/​EffectCatalogThumbnailTests.swift Verifies description-only catalog updates publish state without preparation.
Tests/​AppRouterTests/​EffectThumbnailDecodingTests.swift Expands coverage for bundled artwork, colors, zero-size images, and pixel limits.
Sources/​Presenters/​EffectPickerText.swift Applies ready-state artwork checks and scalar-based description formatting; an unbounded suffix scan remains under review.
Sources/​Entity/​EffectThumbnail.swift Updates thumbnail decoding and fallback documentation.
Sources/​AppRouter/​EffectThumbnailDecoding.swift Documents decoder behavior and pixel limits; overflow-safe dimension multiplication remains required.
README.md Updates description and thumbnail behavior documentation.
Package.swift Updates the AppRouter responsibility comment.
docs/​ARCHITECTURE.md Synchronizes picker, decoder, testing, and architecture documentation.
AGENTS.md Updates guidance for thumbnail limits and decoder placement.
.claude/​rules/​testing.md Documents the bundled-thumbnail decoder fixture.
.claude/​rules/​dev-verification.md Adds malformed-thumbnail verification instructions.
.claude/​rules/​architecture-boundaries.md Documents the AppRouter thumbnail-decoding responsibility.
.claude/​CLAUDE.md Updates the architecture chain to include picker thumbnail decoding.

Review effort: Lite

Comment on lines +14 to +15
/// thumbnail over the limit is not drawn and the cell draws its tile. Counted in pixels, not in
/// points: a Retina export says it is half as wide as it is.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

agent reply

ご指摘のとおりです。pixelsWide * pixelsHigh は、ヘッダが UInt32.max 近い辺を宣言すると Swift でトラップします(手元の測定では、そのような PNG は NSBitmapImageRep が nil を返しますが、それに頼る理由はありません)。上限の判定を width <= maxPixels / height に直し、0 の辺も同じ guard で弾くようにしました。c5f9689 で修正しています。

let content = description.unicodeScalars.drop { $0.properties.isWhitespace }
let head = String(Substring(content.prefix(helpDescriptionLimit))).trimmingCharacters(in: .whitespacesAndNewlines)
guard !head.isEmpty else { return nil }
return content.dropFirst(helpDescriptionLimit).contains { !$0.properties.isWhitespace } ? head + "…" : head

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

agent reply

ご指摘のとおり、切り口のあとの空白を最後まで読む実装で、コメントの「先頭だけを読む」と食い違っていました。切り口のあと 280 スカラーまでしか見ないようにしました(b39fd97)。長い空白の隙間のあとの文章は、省略記号なしで隠れます。この境界はコメント・ARCHITECTURE・テストの 1 ケースに書きました。

一方、説明を Presenter に届く前に切る案は採っていません。上限は表示の方針なので Presenter に置き、カタログの値は manifest のまま渡す、という #69 の判断を変えないためです。また、本文の前にある空白は、読み飛ばさないと本文が隠れてしまうため、引き続き読み飛ばします。

@codecov

codecov Bot commented Nov 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

pixelsWide * pixelsHigh traps in Swift when a header claims sides near UInt32.max, so a crafted thumbnail could crash the picker instead of drawing the tile. The limit is now width <= maxPixels / height, with zero sides refused in the same guard.
The ellipsis check scanned the rest of the description until a non-whitespace scalar, so a description padded with whitespace after the cut was read through on every keystroke. It now looks at no more than 280 scalars past the cut, which the comment, ARCHITECTURE and a test case state, including that a longer gap hides what follows it without an ellipsis.
@GeneralD
GeneralD merged commit 24bee02 into main Nov 3, 2026
8 checks passed
@GeneralD
GeneralD deleted the fix/pr69-post-merge-review branch November 3, 2026 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants