Skip to content

fix(gui): expose all combo strategies in selector - #3335

Closed
x3M3x wants to merge 3 commits into
lidge-jun:devfrom
x3M3x:codex/fix-combo-strategy-selector
Closed

fix(gui): expose all combo strategies in selector#3335
x3M3x wants to merge 3 commits into
lidge-jun:devfrom
x3M3x:codex/fix-combo-strategy-selector

Conversation

@x3M3x

@x3M3x x3M3x commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Expose every runtime combo strategy in the GUI selector: Failover, Round-robin, Random, Least-used, and Reset-window.
  • Remove the disabled fallback button that appeared when a strategy was selected but not offered by the selector.
  • Add a regression test covering all five enabled radio buttons.

Verification

  • bun test gui/tests/combo-strategy-selector.test.tsx
  • bun test gui/tests/combo-roundtrip.test.tsx
  • bun run build:gui
  • bun run typecheck

Combo strategy selector with all five strategies

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

4/4 boxes ticked.

This pull request has been marked Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

Hygiene

Deterministic PR hygiene checks passed.

@github-actions
github-actions Bot marked this pull request as draft September 3, 2026 06:20
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: a08ea334-8525-41e8-940f-7d4f60909423

📥 Commits

Reviewing files that changed from the base of the PR and between 38f8a81 and 45f3c53.

⛔ Files ignored due to path filters (2)
  • docs/pr-assets/combo-strategy-selector-after.png is excluded by !**/*.png
  • docs/pr-assets/combo-strategy-selector-before.png is excluded by !**/*.png
📒 Files selected for processing (2)
  • gui/src/components/combo-workspace-controls.tsx
  • gui/tests/combo-strategy-selector.test.tsx

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The combo strategy selector now renders all entries from COMBO_STRATEGIES. A new test verifies the five expected labels and confirms that every radio button remains enabled.

Changes

Combo strategy selector

Layer / File(s) Summary
Render configured strategy options
gui/src/components/combo-workspace-controls.tsx:4, gui/src/components/combo-workspace-controls.tsx:24-36, gui/tests/combo-strategy-selector.test.tsx:1-31
StrategySeg now generates options from COMBO_STRATEGIES and resolves labels with COMBO_STRATEGY_LABEL_KEYS. The test verifies the Failover, Round-robin, Random, Least-used, and Reset-window labels and confirms that all five radio buttons are enabled.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 45f3c

The combo strategy selector now exposes all five configured strategies and removes the unavailable-strategy fallback, with coverage confirming each option is enabled. No current merge-readiness risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: exposing all combo strategies in the GUI selector. It matches the implementation and the regression test.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 68 / 80

이 PR은 Combos GUI의 전략 선택기(StrategySeg)가 런타임이 이미 지원하는 다섯 가지 전략을 다 고르게 고치는 작은 표시·입력 버그 수정이다. 지금 dev HEAD는 38f8a8164(Cursor 피커 슬러그 보존 #3330)이고, 콤보 라우팅 자체는 예전 #2050에서 failover / round-robin / random / least-used / reset-window 다섯 값이 OcxComboStrategy(src/types/config.ts)와 CLI(src/cli/combo.ts--strategy)에 이미 들어가 있다. GUI 쪽 데이터 모델도 맞춰져 있다. gui/src/combo-workspace-data.tsCOMBO_STRATEGIES·COMBO_STRATEGY_LABEL_KEYS·힌트 키가 다섯을 전부 들고 있고, normalizeStrategy도 그 집합을 본다.

그런데 실제 라디오 UI인 gui/src/components/combo-workspace-controls.tsxStrategySeg는 하드코딩으로 failoverround-robin 두 버튼만 그린다. 설정·CLI로 random / least-used / reset-window가 이미 들어가 있으면, 선택됐지만 고를 수 없는 전략을 비활성 버튼으로만 보여 준다. 그래서 사용자는 GUI에서 그 세 전략으로 바꾸지도 못하고, 이미 그 전략인 콤보를 보면 “고른 것처럼 보이는데 눌러도 안 되는” 상태가 된다. 추가·상세 모달(combo-workspace-add-modal.tsx, combo-workspace-detail-panel.tsx)이 둘 다 같은 StrategySeg를 쓰므로 생성·편집 경로가 한꺼번에 막혀 있다. 이 PR은 그 하드코딩 목록을 지우고 COMBO_STRATEGIES.map으로 라벨 키를 읽게 바꿔, 런타임·타입·i18n과 GUI를 다시 한줄로 맞춘다. 비활성 fallback 버튼도 함께 제거한다. 회귀 테스트 gui/tests/combo-strategy-selector.test.tsx는 영어 로케일에서 라디오 다섯 개·라벨 다섯 개·disabled 없음을 검사한다. types.ts/config.ts 분할 캠페인과는 겹치지 않는 독립 GUI 패치라 닫을 이유가 없다.

라인 단위로 보면 방향은 맞다. 다만 머지 전에 레이아웃과 초안 상태만 한 번 보면 좋다.

라인 gui/src/styles-combos-workspace.css:463-476 - .cwi-strategy-seginline-flex이고 버튼 min-width: 88px다. 전략이 2개일 때는 괜찮았지만 5개가 되면 대략 440px 이상 가로가 필요하다. 좁은 Combos 패널·모바일에서 한 줄이 넘치거나 잘릴 수 있다. 같은 시각의 #3333(모바일 Combos 레이아웃)과 겹칠 수 있으니, 줄바꿈(flex-wrap)이나 더 짧은 라벨/세그먼트 밀도는 메인테이너가 한 번 눈으로 보면 좋다.

라인 gui/tests/combo-strategy-selector.test.tsx:17-30 - 정적 HTML에 라벨·개수·disabled 없음만 본다. value="random"일 때 해당 버튼의 aria-checked="true"와, 다섯 id가 모두 클릭 가능한지는 아직 안 잡는다. 지금 버그를 막는 최소 회귀로는 충분하지만, 나중에 onChange 스파이를 한 줄 넣으면 더 단단해진다.

경로 docs/pr-assets/combo-strategy-selector-*.png - before/after 스크린샷 커밋은 리포에 이미 docs/pr-assets/ 관례가 있어서 금지할 필요는 없다. 다만 머지 필수 자산은 아니고 PR 본문 설명용이다. 용량·노이즈가 거슬리면 본문만 남기고 파일은 빼도 기능에는 영향이 없다.

경로 TargetEditor weight 분기(combo-workspace-controls.tsx 근처 round-robin|random만 weight 입력) - 이 PR이 만진 부분은 아니지만, 전략을 GUI에서 새로 고를 수 있게 되면 least-used / reset-window로 바꾼 뒤 weight가 숨겨지는 기존 분기가 그대로 맞는지 스모크만 하면 된다. 문서·CLI도 그 두 전략에서 weight를 무시한다.

메인테이너의 판단이 필요한 지점

  • 초안(draft)이고 리뷰 readiness 체크리스트가 아직 비어 있다. CI hygiene/enforce-target은 통과했지만 CodeRabbit이 pending이다. “코드는 맞고 초안만 풀면 머지”인지, “모바일 5버튼 레이아웃까지 손보고 머지”인지.
  • 5버튼 세그먼트를 현재 pill 스타일 그대로 둘지, 좁은 폭용 wrap/compact를 이 PR에 넣을지(#3333과 묶을지 단독으로 둘지).
  • docs/pr-assets PNG 두 장을 트리에 남길지.

너의 추천
초안 체크리스트·CI가 초록이 되면 머지 후보로 받아도 된다. 런타임·COMBO_STRATEGIES·i18n과 GUI를 맞추는 명확한 버그 수정이고 범위가 작다. 머지 전에 (1) 좁은 폭에서 5버튼이 깨지지 않는지만 스크린으로 확인하고, (2) readiness 박스를 채운 뒤 draft를 풀면 된다. 레이아웃이 깨지면 CSS wrap만 후속 한 줄로 빼도 이 PR 본체를 막을 필요는 없다. types/config 분할로 무효화되는 패치가 아니므로 close-don't-rebase 대상이 아니다.

이 댓글은 grok-bot이 작성했습니다

@github-actions
github-actions Bot marked this pull request as ready for review September 3, 2026 06:24
lidge-jun added a commit that referenced this pull request Sep 3, 2026
Carried from #3335 onto current dev. Independent of the other carried
fixes, so it ships as its own PR rather than a stack layer.

Co-authored-by: jun <jun@lidge.dev>
Co-authored-by: x3M3x <amroeid1999@gmail.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Landed via #3373 at d753fa5

1 similar comment
@lidge-jun

Copy link
Copy Markdown
Owner

Landed via #3373 at d753fa5

@lidge-jun lidge-jun closed this Sep 3, 2026
@lidge-jun lidge-jun added the landed-via-maintainer Original PR closed after landing via a maintainer merge train label Sep 3, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev as #3373 (d753fa5), carried onto current dev with your Co-authored-by trailer. One follow-up worth knowing: five buttons at min-width 88px in a non-wrapping container need roughly 454px, and the modal can be narrower — a mobile visual check is warranted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working landed-via-maintainer Original PR closed after landing via a maintainer merge train review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants