-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(cli): preserve slash command names in narrow terminals #8657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -237,7 +237,7 @@ export function SuggestionsDisplay({ | |
| : { flexShrink: 1 as const })} | ||
| > | ||
| <Box> | ||
| {labelElement} | ||
| <Box flexShrink={0}>{labelElement}</Box> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] With the label now wrapped in 中文说明标签被包进 — qwen3.8-max via Qwen Code /review (v0.21.7)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the detailed measurements. I agree that long-name and extremely narrow terminal combinations deserve a bounded fallback. A robust solution likely needs an explicit remaining-width policy that truncates or hides hints and badges, plus a width-matrix test suite, rather than another isolated flex adjustment. That is broader than the focused command-name truncation fixed here, and rendering at those widths was already degraded before this change. With CI green and the focused fix already approved, I would prefer to defer that broader narrow-width policy to a follow-up. |
||
| {suggestion.argumentHint && ( | ||
| <Text color={theme.text.secondary}> | ||
| {' '} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The new test pins only the name-survival half of the fix: it never asserts that the argument hint wraps and stays visible, which is this PR's second stated behavior ("Argument hints continue to yield space by wrapping"). The sibling reverse-mode test asserts both sides of its tradeoff (reference intact AND description truncated); this one asserts only one side. — Failure scenario: a future change that renders the hint with
wrap="truncate-end"(or skips it when the column overflows) keeps the command names intact, so this test stays green while the wrap-instead-of-vanish behavior silently regresses. Verified by probe: under a simulated truncate mutation this test file still passed 10/10 while the frame no longer contained the hint tail ([--fix]).中文说明
新测试只固定了修复中"命令名存活"这一半:它没有断言参数提示换行后仍然可见——而这是本 PR 明确承诺的第二个行为("参数提示仍可通过换行让出空间")。同文件中 reverse 模式的姊妹测试同时断言了权衡的两个方面(引用完整保留且描述被截断),而本测试只断言了一方面。— 故障场景:未来若有改动把提示渲染为
wrap="truncate-end"(或在列溢出时直接跳过提示),命令名依然完整,本测试仍然通过,但"换行而不是消失"的行为会悄无声息地回归。已用探针验证:在模拟的截断变异下,整个测试文件仍 10/10 通过,而渲染帧中已不再包含提示尾部([--fix])。建议补充对换行后提示内容的断言,例如expect(lastFrame()).toContain('[--fix]');——已验证在当前 HEAD 通过、在上述截断变异下失败。— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. I agree this would strengthen a broader contract around hint visibility. This PR is intentionally scoped to the reported defect: the actionable command name must not lose characters. Argument hints are secondary and may legitimately be wrapped, truncated, or hidden by a future narrow-width policy, so asserting the full
[--fix]tail would broaden the behavior guaranteed here. CI is green and the focused change has already received an approval, so I would prefer to keep this PR small and handle hint-visibility guarantees in a follow-up.