fix(combos): preserve all five combo strategies in dashboard and docs - #2929
Conversation
PR lidge-jun#2050 added random, least-used, and reset-window strategies, but the dashboard parser still collapsed them to failover, so an untouched save silently rewrote the configured strategy and stripped weights that random honors. Parse and PUT now round-trip all five runtime strategies, weights are serialized for round-robin and random, and the strategy picker shows the preserved value when it is outside the two quick options. Also documents all five strategies, weight, and stickyLimit scoping in the English combos guide and routing reference. Validation: focused bun test (6 combo test files, 20 pass incl. 4 new), gui lint:i18n, gui build (tsc -b + vite), docs-site build.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR expands combo routing from two strategies to five. It updates GUI strategy handling, random weight serialization and validation, round-trip tests, and documentation for the new routing behavior. ChangesCombo strategy expansion
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
1/4 boxes ticked. Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked. |
리뷰 · 우선순위 61 / 80이 PR은 대시보드가 콤보를 저장할 때 전략을 몰래 바꾸는 버그를 고친다. 지금 그런데 대시보드 쪽은 아직 옛 두 칸만 안다. 고치는 내용은 그 침묵 재작성만 먼저 막는다. 타입을 다섯 개로 맞추고, 파서가 모르는 값만 이 범위는 점수는 침묵 재작성이 실제 설정 손실이라 중간보다 조금 높다. 초안이고 레일 분류·번역·생성 UI 가 빠졌으므로 지금 합치지는 않는다. 라인 - gui/src/combo-workspace-data.ts groupCombos (199–206행) - round-robin이 아니면 전부 failover 칸에 넣는다. 고친 뒤에도 random/least-used/reset-window 콤보가 장애 조치 목록과 숫자에 섞인다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Verified independently at head 4 pass / 0 fail restored, and The part I checked hardest was whether the widened type is correct rather than merely wider. Rendering a preserved out-of-band strategy as a disabled chip is the right call over silently relabeling it. A picker that cannot represent a value should say so, not lie about it. One note for the record rather than a change request: Merging once the readiness checklist is complete and CI is green — the two unticked boxes are yours to confirm (local CI green, and Codex/CodeRabbit findings resolved). I did not tick them for you. |
1 similar comment
|
Verified independently at head 4 pass / 0 fail restored, and The part I checked hardest was whether the widened type is correct rather than merely wider. Rendering a preserved out-of-band strategy as a disabled chip is the right call over silently relabeling it. A picker that cannot represent a value should say so, not lie about it. One note for the record rather than a change request: Merging once the readiness checklist is complete and CI is green — the two unticked boxes are yours to confirm (local CI green, and Codex/CodeRabbit findings resolved). I did not tick them for you. |
|
Merging this as maintainer rather than waiting on the remaining checklist boxes, and saying plainly why. Three boxes are unticked: local CI green, Codex/CodeRabbit findings resolved, and ready-for-review. The first is an author attestation the gate cannot verify for a fork contributor — you cannot start repository CI, so it exists on trust either way. The second has nothing outstanding: there are no reviews and no unresolved inline findings on this PR. And repository CI is green on What made me comfortable is that I verified the substance rather than the process: reverting Thanks for the follow-up on #2050 — silently rewriting a saved strategy is the kind of bug users experience as "the dashboard corrupted my config," and the disabled-chip choice for out-of-band strategies is better than relabeling. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Thanks for the merge and for the independent verification. The UI gaps named in the Korean review above were still open at merge head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14cb43b0ce
ℹ️ 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".
| return typeof raw === "string" && COMBO_STRATEGY_SET.has(raw) | ||
| ? raw as ComboStrategy |
There was a problem hiding this comment.
When /api/combos returns random, least-used, or reset-window, this normalization now preserves the value, but groupCombos() still places every non-round-robin item in its failover bucket; consequently, OverviewPanel reports all three advanced strategies as Failover. Add strategy-specific buckets/counts or otherwise exclude these values from the failover total so the dashboard reflects the management configuration model.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| type="button" | ||
| role="radio" | ||
| aria-checked={true} | ||
| className="btn btn-sm btn-primary" | ||
| disabled |
There was a problem hiding this comment.
For a combo using random, least-used, or reset-window, the only radio marked checked is unconditionally disabled, so keyboard focus skips the current selection and lands on an unchecked quick option instead. Render the preserved selection as a focusable checked radio (using aria-disabled if it must remain read-only) so keyboard and assistive-technology users can reach and identify the active strategy.
AGENTS.md reference: gui/AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
| | `targets[].weight` | No | `1` | Integer from 1 to 10,000. Used by round-robin and random; ignored by failover, least-used, and reset-window. | | ||
| | `strategy` | No | `"failover"` | `"failover"`, `"round-robin"`, `"random"`, `"least-used"`, or `"reset-window"`. | |
There was a problem hiding this comment.
The canonical table now documents five strategies, but all seven translated combo guides and translated routing references still explicitly say that only failover and round-robin are valid and that weights apply only to round-robin. Users browsing those locales therefore receive configuration guidance that contradicts both this page and the runtime; update the directly affected localized tables and strategy descriptions alongside the English source.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
…lidge-jun#2929) PR lidge-jun#2050 added random, least-used, and reset-window strategies, but the dashboard parser still collapsed them to failover, so an untouched save silently rewrote the configured strategy and stripped weights that random honors. Parse and PUT now round-trip all five runtime strategies, weights are serialized for round-robin and random, and the strategy picker shows the preserved value when it is outside the two quick options. Also documents all five strategies, weight, and stickyLimit scoping in the English combos guide and routing reference. Validation: focused bun test (6 combo test files, 20 pass incl. 4 new), gui lint:i18n, gui build (tsc -b + vite), docs-site build.
2 participants
Summary
random,least-used, andreset-windowstrategies tofailover, so opening a combo in the workspace and saving it untouched silently rewrote its strategy and stripped the weights thatrandomhonors.round-robinandrandom, and the strategy picker renders a preserved strategy that is outside the two quick options as a disabled chip instead of mislabeling it.weightandstickyLimitscoping in the English combos guide and the routing reference.Preserved
randomstrategy in the combo workspaceVerification
14cb43b0c, latestdev):bun test ./gui/tests/combo-strategy-roundtrip.test.ts— 4 pass;cd gui && bun run build(tsc -b + vite) passes.cd gui && bun run lint:i18n, and the docs-site build all passed.Checklist
No authentication, credential, or workflow surface is touched by this change.
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Summary by CodeRabbit
New Features
Documentation
Tests