refactor(rust): bookmark 検出ロジックを lib-jj-helpers クレートに集約 (PR-C) - #58
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthrough新しい共有ライブラリクレート Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (e.g., cli-pr-monitor)
participant Lib as lib-jj-helpers
participant JJ as `jj` (subprocess)
participant AppLog as caller_log_fn
CLI->>Lib: get_jj_bookmarks(stderr_mode, fallback_log)
Lib->>JJ: run `jj log -r <revset>` (for each revset until hit)
JJ-->>Lib: stdout / stderr / exit status
alt stderr captured and non-empty
Lib->>AppLog: call log fn with stderr
end
Lib-->>CLI: Vec<String> (bookmarks)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/adr/adr-024-shared-jj-helpers-library.md (1)
53-87:⚠️ Potential issue | 🟡 Minor公開 API 一覧を PR-C の実装内容に合わせてください。
Line 55-57 では
capture_commit_id/diff_is_emptyが公開 API として残っていますが、Line 87 では未移設と説明されています。また、実装側のget_jj_bookmarksはfallback_logも受け取るため、Line 65 の signature も古いです。📝 修正案
-公開する API の初期セット: - -- ADR-021 原則 1-4 系 (変更検出): - - `capture_commit_id() -> Option<String>` - - `diff_is_empty(from: &str, to: &str) -> bool` -- ADR-021 原則 5 系 (bookmark 検出): - - 定数 `BOOKMARK_SEARCH_REVSETS = ["@", "@-", "@--"]` - - 定数 `TRUNK_BOOKMARKS = ["main", "master", "trunk", "develop"]` - - `is_trunk_bookmark(name: &str) -> bool` - - `parse_bookmark_list_output(stdout: &str) -> Vec<String>` - - `select_from_revsets(...)` (クロージャ注入型 pure function) - - `query_bookmarks_at(revset: &str) -> Vec<String>` - - `get_jj_bookmarks(stderr_mode: StderrMode) -> Vec<String>` +PR-C で公開する API の初期セット: + +- ADR-021 原則 5 系 (bookmark 検出): + - 定数 `BOOKMARK_SEARCH_REVSETS = ["@", "@-", "@--"]` + - 定数 `TRUNK_BOOKMARKS = ["main", "master", "trunk", "develop"]` + - `is_trunk_bookmark(name: &str) -> bool` + - `enum StderrMode { Silent, Piped(fn(&str)) }` + - `parse_bookmark_list_output(stdout: &str) -> Vec<String>` + - `query_bookmarks_at(revset: &str, stderr_mode: &StderrMode) -> Vec<String>` + - `select_from_revsets(..., fallback_log: Option<fn(&str)>) -> Vec<String>` + - `get_jj_bookmarks(stderr_mode: StderrMode, fallback_log: Option<fn(&str)>) -> Vec<String>`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/adr-024-shared-jj-helpers-library.md` around lines 53 - 87, The public API list is out of sync with the implementation: remove capture_commit_id and diff_is_empty from the advertised public API (they remain only in cli-pr-monitor per the note) and update the get_jj_bookmarks signature to reflect it accepts a fallback_log (and any injected log closure) and StderrMode; specifically, edit the API section to (1) omit capture_commit_id() and diff_is_empty() from the initial public API list, (2) change the bookmark-related signature line to include the actual parameters used by the implementation (e.g., get_jj_bookmarks(stderr_mode: StderrMode, fallback_log: impl Fn(&str) or equivalent description), and (3) ensure references to StderrMode and log injection match the earlier “stderr handling is parameterized” description so the doc matches the code for get_jj_bookmarks, query_bookmarks_at, and any functions that accept the injected log closure.docs/todo.md (1)
172-186:⚠️ Potential issue | 🟡 MinorPR-C の状態表記が open PR と矛盾しています。
Line 173 では PR-C を「本 PR」としていますが、Line 186 では「merged/本 PR」となっています。この PR はまだ open なので、merge 前の todo としては
mergedを外した方が安全です。📝 修正案
-> └── PR-C (jj-helpers 抽出, merged/本 PR) +> └── PR-C (jj-helpers 抽出, 本 PR)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/todo.md` around lines 172 - 186, The PR status for PR-C is inconsistent; update the two occurrences of the PR-C description so they both reflect that this is the current open PR (remove the "merged" label). Specifically, edit the text instances "PR-C (jj-helpers 抽出 / 本 PR)" and "PR-C (jj-helpers 抽出, merged/本 PR)" so they consistently indicate the PR is open (e.g., "PR-C (jj-helpers 抽出 / 本 PR)" or "PR-C (jj-helpers 抽出, open)"), ensuring the PR dependency block no longer lists PR-C as merged.docs/adr/adr-021-jj-change-detection-principles.md (1)
133-155:⚠️ Potential issue | 🟡 Minor完了済みの共通ライブラリ化が「次ステップ」に残っています。
Line 133 で
src/lib-jj-helpers/へ集約済みと更新されていますが、Line 155 ではまだ「切り出しを検討」「ADR-024 (仮) で観察中」となっており、読者に現在状態を誤認させます。📝 修正案
-- **共通ライブラリ化の検討**: 2 つ目の使用例が出たら `src/lib-jj-helpers/` への切り出しを検討 (ADR-024 (仮) で観察中) +- **lib-jj-helpers の利用徹底**: 新規 jj 連携クレートでは `src/lib-jj-helpers/` を依存に追加し、ADR-021 原則 5 の共通 API を利用する🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/adr-021-jj-change-detection-principles.md` around lines 133 - 155, ドキュメント内で `src/lib-jj-helpers/` へ既に集約済みと書かれている一方で「次ステップ」に `共通ライブラリ化の検討` や `ADR-024 (仮)` と残っており状態が矛盾していますので、`次ステップ` セクションから「共通ライブラリ化の検討」および `ADR-024 (仮)` の文言を削除するか、`src/lib-jj-helpers/` への切り出しが完了している旨（`ADR-024` を本採用済みとして言及）に書き換え、該当箇所（`次ステップ` セクション内の「共通ライブラリ化の検討」文言および `ADR-024 (仮)` 表記）を更新してください。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/adr/adr-021-jj-change-detection-principles.md`:
- Around line 133-155: ドキュメント内で `src/lib-jj-helpers/` へ既に集約済みと書かれている一方で「次ステップ」に
`共通ライブラリ化の検討` や `ADR-024 (仮)` と残っており状態が矛盾していますので、`次ステップ` セクションから「共通ライブラリ化の検討」および
`ADR-024 (仮)` の文言を削除するか、`src/lib-jj-helpers/` への切り出しが完了している旨（`ADR-024`
を本採用済みとして言及）に書き換え、該当箇所（`次ステップ` セクション内の「共通ライブラリ化の検討」文言および `ADR-024 (仮)`
表記）を更新してください。
In `@docs/adr/adr-024-shared-jj-helpers-library.md`:
- Around line 53-87: The public API list is out of sync with the implementation:
remove capture_commit_id and diff_is_empty from the advertised public API (they
remain only in cli-pr-monitor per the note) and update the get_jj_bookmarks
signature to reflect it accepts a fallback_log (and any injected log closure)
and StderrMode; specifically, edit the API section to (1) omit
capture_commit_id() and diff_is_empty() from the initial public API list, (2)
change the bookmark-related signature line to include the actual parameters used
by the implementation (e.g., get_jj_bookmarks(stderr_mode: StderrMode,
fallback_log: impl Fn(&str) or equivalent description), and (3) ensure
references to StderrMode and log injection match the earlier “stderr handling is
parameterized” description so the doc matches the code for get_jj_bookmarks,
query_bookmarks_at, and any functions that accept the injected log closure.
In `@docs/todo.md`:
- Around line 172-186: The PR status for PR-C is inconsistent; update the two
occurrences of the PR-C description so they both reflect that this is the
current open PR (remove the "merged" label). Specifically, edit the text
instances "PR-C (jj-helpers 抽出 / 本 PR)" and "PR-C (jj-helpers 抽出, merged/本 PR)"
so they consistently indicate the PR is open (e.g., "PR-C (jj-helpers 抽出 / 本
PR)" or "PR-C (jj-helpers 抽出, open)"), ensuring the PR dependency block no
longer lists PR-C as merged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5471c0f8-cc3b-46ad-abb7-507d9197d620
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomldocs/adr/adr-021-jj-change-detection-principles.mddocs/adr/adr-024-shared-jj-helpers-library.mddocs/adr/adr-028-pnpm-create-pr-gate.mddocs/todo.mdsrc/cli-merge-pipeline/Cargo.tomlsrc/cli-merge-pipeline/src/main.rssrc/cli-pr-monitor/Cargo.tomlsrc/cli-pr-monitor/src/util.rssrc/cli-push-runner/Cargo.tomlsrc/cli-push-runner/src/stages/push_jj_bookmark.rssrc/lib-jj-helpers/Cargo.tomlsrc/lib-jj-helpers/src/lib.rs
cli-pr-monitor / cli-merge-pipeline / cli-push-runner の 3 クレートで重複していた bookmark 検出ロジックを ADR-024 (本採用) に従い `src/lib-jj-helpers/` に集約。
## 新クレート
- `src/lib-jj-helpers/` 新設 (ADR-012 命名規約、ADR-026 workspace 準拠)
- 公開 API:
- 定数: `TRUNK_BOOKMARKS`, `BOOKMARK_SEARCH_REVSETS`
- 関数: `is_trunk_bookmark`, `parse_bookmark_list_output`, `query_bookmarks_at`, `select_from_revsets`, `get_jj_bookmarks`
- 型: `StderrMode { Silent, Piped(fn(&str)) }`
- 14 unit tests (fallback_log コールバック検証 2 件を新規追加)
## 設計方針
- **stderr ハンドリングを `StderrMode` で選択**: cli-pr-monitor は `Silent` (CI ログ汚染回避)、cli-merge-pipeline は `Piped(log_info)` (診断情報を出す)
- **log 関数は `fn(&str)` ポインタで注入**: 各クレート固有 prefix (`[post-pr-monitor]` / `[merge-pipeline]`) を崩さない
- **fallback_log は `Option<fn(&str)>`**: `@-` や `@--` で hit した場合のみ通知 (noise 抑制)
## 呼び出し側差し替え
- cli-pr-monitor (`util.rs`): `get_jj_bookmarks()` を lib 呼び出しに置換、重複テスト削除
- cli-merge-pipeline (`main.rs`): 同上、stderr は `Piped` で継続
- cli-push-runner (`push_jj_bookmark.rs`): `is_trunk_bookmark` のみ lib 借用、他ロジックは crate-local 保持 (`parse_bookmark_list_output` は `jj bookmark list` 出力用で semantics が異なる)
## 検証
- cargo test --workspace: 363 tests PASS (1 ignored)
- pnpm build:all: 全 9 exe ビルド成功
- cargo clippy: modified 4 クレートに warning なし
- 行数インパクト: +425 / -492 = net -67 行 (重複テスト集約効果)
## ADR 更新
- ADR-024: 実装フェーズを「実施済」に反映、`capture_commit_id` / `diff_is_empty` は将来 PR で段階的移設と明記
- ADR-021 原則 5: 「3 クレートで重複 → lib-jj-helpers に集約済」と完了反映
- ADR-028: PR-D 参照番号を #7 に更新 (PR-B/PR-C 完了で docs/todo.md がリナンバー)
refs: ADR-024, ADR-021, PR #56 (PR-A), PR #57 (PR-B), PR #54, PR #55
aloekun
force-pushed
the
refactor/jj-helpers-crate
branch
from
April 19, 2026 03:30
5b4e3ae to
f3d6b08
Compare
3 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/lib-jj-helpers/新規クレート: bookmark 検出の共通 primitives (ADR-024 本採用)TRUNK_BOOKMARKS/BOOKMARK_SEARCH_REVSETS)、関数 5 個、StderrModeenumContext
ADR-024 の正式採用条件 (3 箇所 port 完了) を PR-A 時点で満たしており、本 PR で実抽出。
PR #55 の CodeRabbit Nitpick で指摘されていた 3 クレート重複の解消を兼ねる。
Design highlights
StderrModeenum +fn(&str)ポインタ注入: cli-pr-monitor はSilent、cli-merge-pipeline はPiped(log_info)で差異を吸収。クロージャ回避で lifetime 不要 +Send + Sync自動fallback_log: Option<fn(&str)>:@-/@--で hit した時のみ通知 (noise 抑制)is_trunk_bookmarkのみ借用: 同名関数でも semantics が違うもの (parse_bookmark_list_outputはjj bookmark list出力向け vsjj logtemplate 出力向け) は lib に入れず crate-local 保持Validation
cargo test --workspace: 363 tests PASS (1 ignored、既存)pnpm build:all: 全 9 exe ビルド成功cargo clippy: modified 4 クレートに warning なしADR updates (completion reflection)
capture_commit_id/diff_is_emptyは 2 つ目の使用例出現時に段階的移設と明記docs/todo.md #7に更新References
Summary by CodeRabbit