Skip to content

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

Merged
lidge-jun merged 1 commit into
devfrom
codex/carry-3335
Sep 3, 2026
Merged

fix(gui): expose all combo strategies in selector#3373
lidge-jun merged 1 commit into
devfrom
codex/carry-3335

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Summary

Carried from #3335 by @x3M3x onto current dev, with the author's Co-authored-by trailer on the commit.

The combo workspace selector hardcoded two strategies while the canonical GUI set in gui/src/combo-workspace-data.ts has five. A combo already saved with any of the other three rendered as a disabled fallback radio — the user could see their own setting but not choose it. The selector now renders the canonical list directly.

combo strategy selector after

Before, for comparison:

combo strategy selector before

Closes #3335

Verification

cd gui && bun test tests/combo-strategy-selector.test.tsx    1 pass, 0 fail
bun run typecheck                                           exit 0

RED without the fix: with value="random", current code emits three radios (two normal plus one disabled fallback) where the test requires five enabled ones.

Worth a reviewer's eye: five buttons at min-width: 88px in a non-wrapping container need roughly 454px, and the modal can be narrower. Not a blocker for exposing the strategies, but a mobile visual check is warranted.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (Selector exposes existing documented strategies; no doc change needed.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Co-authored-by: x3M3x amroeid1999@gmail.com

Summary by CodeRabbit

  • Bug Fixes

    • Updated the combo strategy selector to consistently display all supported strategies, including Random, Least-used, and Reset-window.
    • Removed the fallback disabled option for unsupported strategy values.
  • Tests

    • Added coverage verifying that all five supported strategies appear as enabled radio options in English.

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: x3M3x <amroeid1999@gmail.com>
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 3, 2026 17:09
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-03T17:14:01.836338Z 389ca6a PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@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

Deterministic PR hygiene checks passed.

@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: 0ced08f1-fd51-4669-ac1c-e717a6c305e0

📥 Commits

Reviewing files that changed from the base of the PR and between 664d80c and 389ca6a.

⛔ 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; 1 remains after this review.


📝 Walkthrough

Walkthrough

StrategySeg now renders all five strategies from COMBO_STRATEGIES with localized labels. The disabled fallback was removed. A regression test verifies that all five radio options are present and enabled.

Changes

Combo strategy selector

Layer / File(s) Summary
Render shared strategies and validate options
gui/src/components/combo-workspace-controls.tsx, gui/tests/combo-strategy-selector.test.tsx
StrategySeg maps over COMBO_STRATEGIES and uses COMBO_STRATEGY_LABEL_KEYS for labels. The fallback disabled button was removed. The test verifies Failover, Round-robin, Random, Least-used, and Reset-window as enabled radio options.

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

Merge Risk: ⚪ Minimal · up to 389ca

The combo selector now exposes all five configured strategies and removes the disabled fallback state for saved configurations. No concrete current-head merge 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: exposing all combo strategies in the GUI selector.
Linked Issues check ✅ Passed The PR satisfies issue #3335. gui/src/components/combo-workspace-controls.tsx now maps over COMBO_STRATEGIES, removes the disabled fallback button, and exposes all five canonical strategies. `gui/…
Out of Scope Changes check ✅ Passed The changes are limited to the combo strategy selector and its regression test. Both files directly support issue #3335, with no unrelated cleanup or feature changes shown.
Full details: Linked Issues check

Explanation

The PR satisfies issue #3335. gui/src/components/combo-workspace-controls.tsx now maps over COMBO_STRATEGIES, removes the disabled fallback button, and exposes all five canonical strategies. gui/tests/combo-strategy-selector.test.tsx verifies five enabled radio options. The reported test and typecheck results provide supporting verification.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/carry-3335

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 389ca6a8a7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

["failover", "cws.strategy.failover"],
["round-robin", "cws.strategy.roundRobin"],
] as const).map(([id, key]) => (
{COMBO_STRATEGIES.map((id) => (

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Make the expanded strategy selector responsive

When the dashboard is used on a narrow viewport, rendering all five strategies here makes the selector wider than its container: .cwi-strategy-seg is a non-wrapping inline-flex, every button has min-width: 88px, and the add modal can shrink to 94vw, so the control requires at least 454px before accounting for longer translated labels. This causes horizontal overflow in both the add modal and detail panel, making strategies difficult to reach on mobile; allow the buttons to wrap/use a responsive grid, or switch to a compact select at narrow widths.

Useful? React with 👍 / 👎.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 73 / 80

이 PR은 콤보 워크스페이스의 전략 선택기를, 실제로 런타임이 아는 다섯 가지 전략과 맞춰 줍니다. 지금 dev HEAD(664d80c76, #3367 Logs 비용 칸·겹침 클립 직후)를 보면 gui/src/combo-workspace-data.tsComboStrategyCOMBO_STRATEGIES에는 failover, round-robin, random, least-used, reset-window 다섯 개가 들어 있고, 주석에도 src/types/config.tsOcxComboStrategy와 같다고 적혀 있습니다. 서버 쪽 OcxComboStrategy도 같은 다섯 값입니다. 그런데 gui/src/components/combo-workspace-controls.tsxStrategySeg는 하드코드로 failoverround-robin 두 개만 버튼으로 그리고, 나머지 값이 저장돼 있으면 눌리지 않는(disabled) 가짜 라디오를 하나 더 붙입니다. 그래서 이미 random / least-used / reset-window로 저장된 콤보는 화면에 자기 설정이 보이긴 하지만, 사용자가 그 전략을 고르거나 바꿀 수는 없습니다. 선택지가 막혀 있는 버그입니다.

고치는 방식은 단순합니다. 하드코드 두 줄과 disabled 폴백 버튼을 지우고, COMBO_STRATEGIES.map으로 버튼을 만든 뒤 라벨은 이미 있는 COMBO_STRATEGY_LABEL_KEYS[id]로 읽습니다. 이렇게 하면 카탈로그(combo-workspace-data.ts)에 전략이 하나 더 생겨도 선택기가 같이 따라갑니다. 새 테스트 gui/tests/combo-strategy-selector.test.tsxvalue="random"일 때 라디오가 다섯 개이고, 영어 라벨 Failover / Round-robin / Random / Least-used / Reset-window가 모두 보이며, 어떤 버튼에도 disabled=가 없다는 것을 잠급니다. 본문이 말한 대로, 지금 dev에서는 같은 입력으로 라디오가 세 개(정상 둘 + disabled 폴백 하나)라서 이 테스트는 RED입니다.

원본은 @x3M3x 의 #3335입니다. 이 PR은 그 수정을 현재 dev 위에 올린 carry이고, 커밋에 Co-authored-by: x3M3x가 들어 있습니다. 본문의 Closes #3335는 머지되면 원본 PR을 닫습니다. types.ts / config.ts 분할 캠페인과는 무관합니다. gui/src/combo-workspace-data.tsnormalizeStrategy도 이미 다섯 값을 인정하고, 모르는 값만 failover로 돌립니다. 즉 저장·정규화 쪽은 이미 열려 있고, GUI 선택기만 뒤처져 있던 상태입니다. 추가 모달(combo-workspace-add-modal.tsx)은 StrategySeg 아래에서 COMBO_STRATEGY_HINT_KEYS로 힌트 문장도 보여 주므로, 다섯 전략이 선택기에 나오면 힌트도 자연스럽게 따라갑니다.

CSS 쪽은 gui/src/styles-combos-workspace.css.cwi-strategy-seg .btnmin-width: 88px입니다. 버튼 다섯 개면 대략 454px가 필요하고, 모달(cwi-modal-form)이 그보다 좁으면 줄이 깨지거나 가로로 넘칠 수 있습니다. 본문도 이 점을 적어 두었고, 기능 버그 수정 자체를 막는 정도는 아닙니다. 다만 좁은 화면에서 한 번 눈으로 확인하는 편이 좋습니다. 테스트는 en-US 라벨만 잠급니다. i18n 키 자체는 en/ko/ja/zh 등에 이미 있으므로, 이번 범위에서 다국어 스냅샷까지 넣을 필요는 없습니다.

라인 24-50 (gui/src/components/combo-workspace-controls.tsx, 현재 dev) - 지금은 전략 두 개만 하드코드하고, 나머지 값은 disabled 폴백으로만 보여 줍니다. 이 PR이 그 구간을 COMBO_STRATEGIES 순회로 바꿉니다.

gui/tests/combo-strategy-selector.test.tsx - value="random"일 때 라디오 5개·영어 라벨 5개·disabled 없음을 잠급니다. 현재 dev에서는 RED라서 회귀를 잘 붙잡습니다.

gui/src/styles-combos-workspace.css 471-476 (.cwi-strategy-seg .btn min-width 88px) - 다섯 버튼이 한 줄에 안 들어가면 모달에서 넘칠 수 있습니다. 기능 차단은 아니지만 모바일·좁은 창 확인이 필요합니다.

원본 #3335 - 이 carry가 머지되면 Closes #3335로 닫히는지, 안 닫히면 Landed via #3373 + landed-via-maintainer로 정리해야 합니다.

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

  • 좁은 모달에서 다섯 전략 버튼을 한 줄로 둘지, wrap / 스크롤 / 더 작은 min-width로 줄일지. 이번 PR 범위 밖이어도 바로 이어서 손볼지 여부.
  • #3335를 이 PR 머지와 함께 자동 close에 맡길지, 남는 원본을 수동으로 landed-via로 닫을지.

너의 추천
현재 dev 기준 작은 GUI 정렬 수정이고, 카탈로그·서버 타입과 맞추는 방향이 맞습니다. 테스트도 RED→GREEN으로 잠가 두었으니 머지하세요. 머지 후 #3335가 열려 있으면 Landed via #3373 댓글 + landed-via-maintainer 후 닫으세요. 가능하면 머지 전에 콤보 추가 모달을 좁은 폭에서 한 번만 확인해 주세요.

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

@lidge-jun
lidge-jun merged commit d753fa5 into dev Sep 3, 2026
27 checks passed
@lidge-jun
lidge-jun deleted the codex/carry-3335 branch September 3, 2026 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant