Skip to content

Refactor: Split Ready state — FilteredView, prSelection promotion, conflictedDirs as state#139

Draft
Copilot wants to merge 2 commits into
masterfrom
copilot/refactor-visualizerstate-structure
Draft

Refactor: Split Ready state — FilteredView, prSelection promotion, conflictedDirs as state#139
Copilot wants to merge 2 commits into
masterfrom
copilot/refactor-visualizerstate-structure

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 22, 2026

SnapshotFetchState.Ready recomputed all derived properties (including O(n×m) overlay maps) on every updateReady {} call, even for operations like togglePr that only change PR selection.

Changes

FilteredView — isolate filter-dependent computation

  • New state/FilteredView.kt holds filteredPrs + filteredPrIds, computed from snapshot + FilterState only
  • Ready stores filteredView: FilteredView as a constructor param; filteredPrs/filteredPrIds delegate to it
  • togglePr now does copy(prSelection = ...)filteredView is reused as-is, filter logic skipped
  • updateShowDrafts/updateOnlyMine explicitly recompute filteredView:
// Before: any updateReady{} recomputed filteredPrs
fun updateShowDrafts(value: Boolean) {
    updateReady { copy(filterState = filterState.copy(showDrafts = value)) }
}

// After: only filter-changing ops rebuild FilteredView
fun updateShowDrafts(value: Boolean) {
    updateReady {
        val newFilterState = filterState.copy(showDrafts = value)
        copy(filterState = newFilterState, filteredView = FilteredView.create(snapshot, newFilterState))
    }
}

fun togglePr(prId: String, checked: Boolean) {
    updateReady { copy(prSelection = togglePrSelection(prId, checked)) }  // filteredView untouched
}

prSelection promoted from FilterState to Ready

  • FilterState now only holds showDrafts / onlyMine — no selection concern
  • Ready secondary constructor preserves the old call-site signature

conflictedDirs moved into state

  • computeConflictedDirs moved from ui/shared/OverlayPresentation.ktstate/OverlayState.kt (internal)
  • Ready exposes val conflictedDirs: Set<String> computed once from fileOverlayByPath
  • ExplorerPane, TreemapCanvas, TreemapPane, TreemapViewportModel accept it as a parameter; remember { computeConflictedDirs(...) } calls removed
  • ui/shared/OverlayPresentation.kt deleted

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • dl.google.com
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED -Xmx3072M -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en -Duser.variant -cp /home/REDACTED/.gradle/wrapper/dists/gradle-8.14.3-bin/cv11ve7ro1n3o1j4so8xd9n66/gradle-8.14.3/lib/gradle-daemon-main-8.14.3.jar (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Original prompt

This section details on the original issue you should resolve

<issue_title>stateの整理</issue_title>
<issue_description>- VisualizerState.kt の肥大化

  • Ready stateの分割
  • io.github.hayatoyagi.prvisualizer.ui.shared.computeConflictedDirs の存在
    • 複数のUIコンポーネントから呼び出されるなら、計算結果を state として保持しておいた方が良いのではないか?</issue_description>

Comments on the Issue (you are @copilot in this section)

@HayatoYagi 補足です。

computeConflictedDirs について — 既に remember で保護済み

両方の呼び出し元でキャッシュされています:

  • ExplorerPane.ktremember(root, fileOverlayByPath, directoryOverlayByPath, expandedPaths) の中
  • TreemapCanvas.ktremember(fileOverlayByPath) の中

ただし根本問題は残ります。fileOverlayByPathReadyval として毎回再生成されるため、updateReady {} のたびに remember のキーが変わって無効化されます。computeConflictedDirs の結果を state に持つ案も、fileOverlayByPath が頻繁に変わる限り効果は同じです。


パフォーマンスの最重要ホットパス

最もコストが高いのはチェックボックス操作の経路です:

togglePr(PR チェックボックス1つ)
  → updateReady { copy(filterState = ...) }
  → Ready 再生成
  → filteredPrs 再計算       ← prSelection が変わっただけなのに不要
  → fileOverlayByPath 再計算  ← O(PR数 × ファイル数)、最重量
  → directoryOverlayByPath 再計算

チェックボックスを1つ変えても filteredPrs は変わらないはずなのに再計算される点が、委譲分割の最大の動機です。


委譲案の具体的な方向性

FilterState 側に委譲」という方向は筋が良いと思います:

  • filteredPrsFilterStateallPrs + showDrafts + onlyMine + currentUser に依存)に持たせる
  • selectedPrIds / selectedPrsprSelection に依存する部分として分離
  • fileOverlayByPath / directoryOverlayByPathselectedPrs が変わったときだけ再計算

こうすると togglePr(= prSelection のみ変更)では filteredPrs が再計算されなくなります。computeConflictedDirs の state 化より、この分割の方が効果が大きいはずです。</comment_new>
<comment_new>@HayatoYagi
追記です。

先ほどのコメントで「filteredPrsFilterState に委譲」と書きましたが、そうすると FilterStateallPrsviewerLogin を持つ必要が生じて責務が崩れます。訂正します。


子 state に切り出せるのは、その子 state 内で完結する属性だけ

ポイントは「どの操作で何が変わるか」を整理することです:

操作 変わるもの 変わらないはずのもの
togglePr prSelection filteredPrs(フィルター条件は同じ)
updateShowDrafts filterState
selectDirectory navigationState filteredPrs, overlays
shufflePrColors colorState filteredPrs, overlays

togglePr がホットパスなのに filteredPrs まで毎回再計算されるのが問題の本質です。


FilteredView を切り出す案

filteredPrsfilterState(と snapshot)にだけ依存するネストオブジェクトにまとめると、togglePr の経路で使い回せます:

data class FilteredView(   // filterState + snapshot にだけ依存
    val filteredPrs: List<PullRequest>,
    val filteredPrIds: Set<String>,
)

data class Ready(
    val snapshot: GitHubSnapshot,
    val filteredView: FilteredView,          // togglePr では同じインスタンスを使い回せる
    val prSelection: PrSelection = PrSelection.allVisible(),
    val navigationState: NavigationState = NavigationState(),
    val colorState: ColorState = ColorState(),
) : SnapshotFetchState {
    val selectedPrIds = ...   // prSelection + filteredView にだけ依存
    val selectedPrs = ...
    val fileOverlayByPath = ...
    val directoryOverlayByPath = ...
}

togglePrcopy(prSelection = ...) すると filteredView は同じオブジェクトのまま渡されるため、filteredPrs の再計算が起きません。updateShowDrafts のときだけ FilteredView を再生成する、という形になります。</comment_new>


💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.

…conflictedDirs to state

Co-authored-by: HayatoYagi <26685974+HayatoYagi@users.noreply.github.com>
Agent-Logs-Url: https://github.com/HayatoYagi/PRs_Visualizer/sessions/e3cb1ebd-40f4-4e56-99d2-e2df0580eb03
Copilot AI changed the title [WIP] Refactor VisualizerState to improve performance and maintainability Refactor: Split Ready state — FilteredView, prSelection promotion, conflictedDirs as state Mar 22, 2026
Copilot AI requested a review from HayatoYagi March 22, 2026 06:06
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.

stateの整理

2 participants