feat(gui): clamp long provider notes to two lines and open the full text in a popup - #4328
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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. |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 50 / 80설명 이 PR은 Providers 대시보드의 프로바이더 추가 모달에서, 카탈로그 행에 붙은 긴 note를 두 줄로 줄이고 나머지는 팝업으로 읽는 R2(note clamp) 구현이다. 지금 왜 필요한지부터 말하면, 중요한 설계 선택은 세 가지다. 첫째, reveal는 행 버튼의자식이 아니라 검증 쪽은
메인테이너의 판단이 필요한 지점
너의 추천 CI(특히 gui 테스트 잡)가 초록이 되면, #4324와 #4325가 이 댓글은 grok-bot이 작성했습니다 |
![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: 414ec2dffe
ℹ️ 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".
| export function noteNeedsReveal(note: string | undefined): boolean { | ||
| return (note?.trim().length ?? 0) > NOTE_CLAMP_CHARS; |
There was a problem hiding this comment.
At the normal catalog width, character count does not correspond to the two-line clamp because the same line also contains the adapter chip and the badges reduce the available width. The committed screenshot already demonstrates this for NVIDIA NIM: its 86-character note is visibly cut off after “API...” while this predicate returns false, so the user gets no way to read the remaining key instructions. Detect rendered overflow (or conservatively provide the reveal for every nonempty clamped note) instead of using the note-only length.
Useful? React with 👍 / 👎.
| useEffect(() => { | ||
| const dialog = dialogRef.current; | ||
| if (dialog && !dialog.open) dialog.showModal(); | ||
| }, []); |
There was a problem hiding this comment.
When the popup is dismissed via Escape, the backdrop, or either Close button, onClose immediately unmounts the still-open native dialog; no path calls dialog.close() and the parent retains no reference to the reveal button. Because the focused control is removed with the dialog, keyboard focus falls back to the document instead of returning to “Show full description,” so keyboard users lose their place in the catalog. Close the dialog during teardown and explicitly restore focus to the triggering reveal after the popup state clears.
AGENTS.md reference: gui/AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
…ext in a popup One catalog note is a paragraph. The opencode-free note runs ~1100 characters and meta-muse is longer still, so at the modal width a single row rendered 15-20 lines inside a 360px scroller and became the only row anyone could see. Notes now clamp to two lines and rows with more to show grow a reveal strip along their bottom edge. The reveal is a SIBLING of the row button inside a new .provider-catalog-row-wrap, never a child: .list-row is itself a <button>, a button nested in a button is invalid HTML the parser may hoist out of the row, and stopPropagation cannot save a control that is no longer where it was written. The popup is a native <dialog> opened with showModal(), the same shape as OAuthTosWarningModal, rendered as a fragment sibling of the add-provider overlay rather than inside it. Native showModal is what restores focus to the reveal button on close. AddProviderModal owns the open state because its Escape listener is on window and does not read defaultPrevented, so every stacked overlay has to be named in that guard or Escape closes the whole modal out from under it. Escape now closes the note first and the add-provider modal second. The reveal strip is qualified as .link-btn.provider-catalog-note-more, and that is not redundant: this stylesheet is @imported at the top of styles.css while .link-btn is declared far below at equal specificity, so the bare class lost its background, border and padding and rendered as a full-width underlined link floating between rows. Vite HMR hid it; a production bundle would not have.
414ec2d to
e3fdf8f
Compare
…vidence Four dependent PRs with their exact heads and live CI run ids, plus the two traps that make a green head look red: a cancelled duplicate run leaves a FAILURE ci row attached to the same SHA as the live green run, and a cancelled required check is not a passing one - all three enforce-target attempts on #4328 were cancelled by concurrency, which left the PR UNSTABLE until one was re-run.
1 participant
Summary
opencode-freeruns ~1100 characters andmeta-museis longer still, so at the modal width a single row rendered 15-20 lines inside a 360px scroller and became the only row anyone could see. Notes now clamp to two lines, and a row with more to show grows a reveal strip along its bottom edge that opens the full text in a popup..provider-catalog-row-wrap, never a child..list-rowis itself a<button>; a button nested in a button is invalid HTML that the parser may hoist out of the row, andstopPropagationcannot save a control that is no longer where it was written.<dialog>opened withshowModal()- the same shape asOAuthTosWarningModal- rendered as a fragment sibling of the add-provider overlay rather than inside it. NativeshowModal()is what restores focus to the reveal button on close.AddProviderModalowns the open state because its Escape listener is onwindowand does not readdefaultPrevented, so every stacked overlay has to be named in that guard or Escape closes the whole modal out from under it. Escape now closes the note first and the add-provider modal second.One trap worth flagging for review: the reveal strip is qualified as
.link-btn.provider-catalog-note-more, and that is not redundant. This stylesheet is@imported at the top ofstyles.csswhile.link-btnis declared far below at equal specificity, so the bare class lost its background, border and padding and rendered as a full-width underlined link floating between two rows. Vite HMR hid it by injecting the edited file last; a production bundle would not have..list-row.provider-catalog-account-row--waitingalready carries the same qualification for the same reason.This is a gui change. Clamped rows with the reveal strip, and the popup:
Verification
bun x tsc --noEmit- passed.cd gui && bun x tsc --noEmit -p tsconfig.json- passed (includesgui/tests).bun run structure:check- passed.noteNeedsRevealcoverage for empty and nonempty notes intests/gui/provider-workspace-data.test.ts, andgui/tests/provider-catalog-note-popup.test.tsxfor sibling placement, the reveal not selecting the provider, and the popup carrying the full note.Based on #4325. Retarget to
devonce that lands.Checklist
Integration review
The owner requested merging this chain into dev. Integrating as lidge-jun under MAINTAINERS.md after exact-head remote CI passes; no self-approval is claimed. Local tests remain NOT RUN.
Both Codex review findings are fixed: every nonempty note has a full-text action, and dialog cleanup closes the native dialog and restores the trigger focus. Manually verified using the NVIDIA NIM row and Escape. This layer includes its structure documentation.
Verification target for maintainer integration:
3627b7bcb256cc737de0c4468163f434675792a9. Cross-platform CI: https://github.com/lidge-jun/opencodex/actions/runs/34672266087. Merge is gated on that exact-head run succeeding. Review repairs were verified with a production GUI build and browser checks; the local suite remains NOT RUN.