fix(app): restore prompt popover visibility#525
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR resolves the slash-command popover clipping regression from PR ChangesSlash Popover Portal & ResizeObserver Positioning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request implements a portaled popover system for the prompt input to ensure that @mention and slash command suggestions remain visible above the composer. The changes include introducing a popover container in the session composer, using SolidJS Portals for rendering, and dynamically calculating the popover's position. Feedback was provided regarding the positioning logic in PromptInput, specifically suggesting that the createEffect should not depend on the popover's visibility state to prevent flickering and should align with SolidJS best practices for observer instantiation.
6faa59c to
d207013
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/app/src/components/prompt-input.tsx (1)
314-314: 💤 Low valueConsider extracting positioning constants.
The positioning calculation uses inline numeric literals:
-8for the top offset80for minimum popover height16for the margin in max-height calculationExtracting these to named constants (e.g.,
POPOVER_TOP_OFFSET,POPOVER_MIN_HEIGHT,POPOVER_MARGIN) would improve readability and maintainability if these values need adjustment later.♻️ Example refactor
+const POPOVER_TOP_OFFSET = 8 +const POPOVER_MIN_HEIGHT = 80 +const POPOVER_MARGIN = 16 +const POPOVER_Z_INDEX = 1000 + createEffect(() => { ... const update = () => { const segmentRect = segment.getBoundingClientRect() setPopoverStyle({ position: "fixed", - top: `${segmentRect.top - 8}px`, + top: `${segmentRect.top - POPOVER_TOP_OFFSET}px`, left: `${segmentRect.left}px`, right: "auto", bottom: "auto", width: `${segmentRect.width}px`, - "max-height": `${Math.max(80, segmentRect.top - 16)}px`, + "max-height": `${Math.max(POPOVER_MIN_HEIGHT, segmentRect.top - POPOVER_MARGIN)}px`, transform: "translateY(-100%)", - "z-index": 1000, + "z-index": POPOVER_Z_INDEX, }) } ... })Also applies to: 319-319
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/components/prompt-input.tsx` at line 314, Extract the inline numeric literals used for popover positioning into named constants (e.g., POPOVER_TOP_OFFSET for -8, POPOVER_MIN_HEIGHT for 80, and POPOVER_MARGIN for 16) and replace the literals in the popover style calculations (the expression using segmentRect.top, the min-height check, and the max-height calculation) with those constants; define the constants at module scope near the top of packages/app/src/components/prompt-input.tsx so they are easy to find and update, and ensure the computed strings (e.g., `${segmentRect.top - POPOVER_TOP_OFFSET}px`) and numeric comparisons use the constants consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/pages/session/composer/session-composer-region.tsx`:
- Line 241: Remove the redundant type assertion on the ref prop: locate the JSX
where DockSegment (accepting ComponentProps<"div">) is given ref={props.inputRef
as (el: HTMLDivElement) => void} and replace it with a direct pass of the
callback by using ref={props.inputRef}; props.inputRef already has the correct
signature so no cast is needed.
---
Nitpick comments:
In `@packages/app/src/components/prompt-input.tsx`:
- Line 314: Extract the inline numeric literals used for popover positioning
into named constants (e.g., POPOVER_TOP_OFFSET for -8, POPOVER_MIN_HEIGHT for
80, and POPOVER_MARGIN for 16) and replace the literals in the popover style
calculations (the expression using segmentRect.top, the min-height check, and
the max-height calculation) with those constants; define the constants at module
scope near the top of packages/app/src/components/prompt-input.tsx so they are
easy to find and update, and ensure the computed strings (e.g.,
`${segmentRect.top - POPOVER_TOP_OFFSET}px`) and numeric comparisons use the
constants consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 75c26f4a-0e24-4122-81d3-efd103c31059
📒 Files selected for processing (4)
packages/app/e2e/prompt/prompt-slash-open.spec.tspackages/app/src/components/prompt-input.tsxpackages/app/src/components/prompt-input/slash-popover.tsxpackages/app/src/pages/session/composer/session-composer-region.tsx
d207013 to
551e50a
Compare
551e50a to
9bd16a9
Compare
|
Review follow-up summary:
Verification after the final amend:
|
Summary
/and@popovers outside its clipped card bounds.PromptPopoverowned and anchored byPromptInput; this is not a new general overlay primitive.Why
#508 wrapped the composer in
DockCard, whose root usesoverflow: hiddenfor joined-card clipping.PromptPopoverstill rendered inline as an upwardabsolutechild, so typing/or@could update popover state while the suggestion body was clipped outside the card.This fix keeps the original prompt popover positioning model and applies a local overflow opt-out only to the prompt composer
DockCard. OtherDockCardinstances keep their clipping contract. The fix deliberately does not introduce a body-level portal, fixed-position overlay system, or Kobalte migration; those would be broader design work for a later slice.Related Issue
Closes #524
Human Review Status
Pending. AstroHan manually verified the current Electron build: home
/, home@, session/, session@, and multi-segment composer states are functionally normal. Remaining feedback about @mention/file popover visual design is a separate DESIGN.md alignment follow-up, not part of this hotfix.Review Focus
DockCardoverflow is only opted out on the prompt composer card, not globally.PromptPopoverremains anchored insidePromptInputwith the originalmax-h-80 -top-2 -translate-y-fullvisual constraints.Risk Notes
Visible UI change in the prompt composer popover layer. The main risk is prompt-card joined-segment clipping because this card locally opts out of
DockCardoverflow. The change is scoped to the prompt composer wrapper and does not change the sharedDockCardprimitive.No dependency, data, permission, migration, packaging, updater, signing, shell, credential, deletion, generated-content, or local-file behavior changes.
How To Verify
The focused E2E covers session
/open, bare/in the home composer, and@mentionsuggestions. It asserts visible popovers/items, topmost hit-testing viaelementFromPoint, top viewport gap, and non-transparent hover paint.Screenshots or Recordings
Manual Electron check was done against the corrected i524 worktree dev environment. Earlier false negatives came from
node_modulessymlinks resolving@opencode-ai/appback to the main checkout; this was corrected locally before final manual verification.Checklist
dev, and my PR title and commit messages use Conventional Commits in English