Conversation
- Create PageSatisfactionBanner with thumbs up/down/comment icons - Add feedback modals for positive/negative ratings with reason selection - Implement API integration with feedbackService.submitFeedback - Display dynamic page name based on current route - Include dark mode support and responsive design Closes #3480
- Change onSubmit return type to Promise<boolean> to handle submission success/failure - Only close modal and clear inputs on successful submission - Keep modal open on failure to allow retry with existing input - Fix textarea label from "this recommendation" to "this page" for clarity - Remove setModalType(null) from finally block to prevent premature closure Fixes code-review feedback from CodeRabbit
- Import PageSatisfactionBanner component from feedback module - Render banner at bottom of main content area after children - Banner now appears consistently on all authenticated Vertex pages - Addresses issue #3480 acceptance criteria for banner placement
- Fix category field: changed from 'page_satisfaction' to 'other' to match API requirements (general, bug, feature_request, performance, ux_design, other) - Replace textarea with ReusableInputField component for consistency with FeedbackLauncher - Replace thumb buttons with ReusableButton component with text variant - Fix banner positioning: removed horizontal padding (px-0) on outer container for full-width layout like Google Cloud example - Updated inner container to use w-full with responsive padding (px-4 sm:px-6) - Removed max-w-4xl constraint to allow banner to span full width - Added proper spacing and alignment for Google Cloud-style layout
- Replaced toast notifications with useBanner hook for consistent UX - Fixed banner to stick to bottom of page with fixed positioning - Removed horizontal spacing (px-0) for full-width banner - Added fixed bottom-0 left-0 right-0 for sticky footer behavior - Banner now displays success/error messages via banner component
… boundaries - Changed from `fixed bottom-0 left-0 right-0` to `sticky bottom-0` to stay within main content area - Removed `w-full` from outer div as it's already full-width by default - Removed `w-full` from inner div to prevent overflow - Banner now respects sidebar/layout boundaries while still sticking to bottom - Maintains edge-to-edge positioning within the content region only
Contributor
📝 WalkthroughWalkthroughThis PR adds a page satisfaction feedback banner that prompts users to rate page quality with positive/negative actions or open a feedback dialog. Selecting positive/negative opens a modal for selecting a predefined reason and optional message, then submits feedback via an API call. The implementation includes banner infrastructure refactoring and layout integration changes. ChangesPage Satisfaction Feedback Banner
Sequence DiagramsequenceDiagram
participant User
participant PageSatisfactionBanner
participant FeedbackModal
participant feedbackService
participant UserContext
User->>PageSatisfactionBanner: Click positive/negative action
PageSatisfactionBanner->>FeedbackModal: Open modal with type
User->>FeedbackModal: Select reason & enter message
User->>FeedbackModal: Click submit
FeedbackModal->>PageSatisfactionBanner: onSubmit(reason, message)
PageSatisfactionBanner->>UserContext: useUserContext (get email)
PageSatisfactionBanner->>feedbackService: submitFeedback(email, rating, reason, metadata)
feedbackService-->>PageSatisfactionBanner: success/error response
PageSatisfactionBanner->>PageSatisfactionBanner: showBanner(success/error)
PageSatisfactionBanner->>FeedbackModal: Close & reset state
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/vertex/components/features/feedback/page-satisfaction-banner.tsx (4)
258-272: 💤 Low valueConsider refactoring to a single conditional FeedbackModal.
Currently, two separate
FeedbackModalinstances are rendered for positive and negative feedback. Since only one can be open at a time (controlled bymodalType), you could simplify to a single modal with conditional props. However, the current approach is clear and works correctly.♻️ Optional refactor to single modal
- <FeedbackModal - isOpen={modalType === 'positive'} - onClose={() => setModalType(null)} - onSubmit={handleSubmitFeedback} - isSubmitting={isSubmitting} - type="positive" - /> - - <FeedbackModal - isOpen={modalType === 'negative'} - onClose={() => setModalType(null)} - onSubmit={handleSubmitFeedback} - isSubmitting={isSubmitting} - type="negative" - /> + {modalType && ( + <FeedbackModal + isOpen={true} + onClose={() => setModalType(null)} + onSubmit={handleSubmitFeedback} + isSubmitting={isSubmitting} + type={modalType} + /> + )}🤖 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 `@src/vertex/components/features/feedback/page-satisfaction-banner.tsx` around lines 258 - 272, Replace the two FeedbackModal renderings with a single FeedbackModal driven by modalType: use isOpen based on modalType (e.g., modalType !== null), pass type using modalType (coerced or defaulted as needed), keep onClose={() => setModalType(null)}, onSubmit={handleSubmitFeedback} and isSubmitting={isSubmitting}; this collapses the duplicated JSX while preserving behavior and opens the correct modal content based on modalType.
184-187: 💤 Low valueDocument why browser user agent is truncated to 80 characters.
The UA string is truncated to 80 chars without explanation. Modern UA strings can be longer than 80 characters, and truncation might lose valuable debugging information (e.g., specific browser version, OS details).
📝 Consider documenting the truncation rationale
browser: typeof window !== 'undefined' - ? navigator.userAgent.slice(0, 80) + ? navigator.userAgent.slice(0, 80) // Truncate to fit API field limit : 'Unknown',🤖 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 `@src/vertex/components/features/feedback/page-satisfaction-banner.tsx` around lines 184 - 187, The user-agent is being truncated via navigator.userAgent.slice(0, 80) when building the browser field—add a short inline comment (or extract the number into a named constant like UA_TRUNCATE_LENGTH) that documents why 80 chars was chosen and the tradeoffs (what is preserved, what may be lost), and consider making the length configurable or removing truncation if full UA is required; update the code around the browser assignment (the browser field and the navigator.userAgent.slice(0, 80) usage) to include this explanatory comment/constant so future readers understand the rationale.
202-202: 💤 Low valueRemove unnecessary blank line.
The empty line before
return trueserves no purpose and can be removed for consistency.🤖 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 `@src/vertex/components/features/feedback/page-satisfaction-banner.tsx` at line 202, Remove the unnecessary blank line in the function around the conditional that currently has an empty line immediately before the `return true` statement in page-satisfaction-banner.tsx; locate the function or method (e.g., the helper that returns a boolean in this component) and delete the stray blank line so `return true` follows the previous statement directly for consistent formatting.
182-182: 💤 Low valueThe pathname fallback to
window.location.pathnameis unnecessary in App Router.Since this component uses
usePathname()from 'next/navigation' (App Router), the pathname is always available as a string during client-side rendering. The fallback is defensive but redundant—no need to keep it.🤖 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 `@src/vertex/components/features/feedback/page-satisfaction-banner.tsx` at line 182, Remove the unnecessary fallback to window.location.pathname in the PageSatisfactionBanner component: where the payload sets page: pathname || window.location.pathname, replace it to use the pathname value directly (page: pathname) since usePathname() from next/navigation always supplies a string in the App Router; update any related references in the same component to rely on pathname only and remove the window global usage.
🤖 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 `@src/vertex/app/changelog.md`:
- Line 8: The changelog entry currently marks the PR as released ("Released: May
17, 2026") even though the PR is open; update the header in
src/vertex/app/changelog.md to an unreleased marker (e.g., "Unreleased" or "Not
released") or remove the release date until the actual release cut so history
stays accurate—locate the line containing the exact text "Released: May 17,
2026" and replace it accordingly.
In `@src/vertex/components/features/feedback/page-satisfaction-banner.tsx`:
- Line 196: The feedback category for the page satisfaction component is too
generic—replace the hardcoded "other" category in the PageSatisfactionBanner
(the object where `category: 'other'` is set) with a specific value such as
"page_satisfaction" or "satisfaction" so backend analytics can distinguish this
feedback type; update the category field used where the component constructs the
feedback payload (look for the object/constant inside PageSatisfactionBanner or
its submit handler) to the chosen specific string and ensure any related type
definitions or telemetry mappings accept that new category name.
- Around line 66-72: The banner is being shown while the modal is still closing
(setTimeout of 150ms), causing overlap with ReusableDialog's exit animation;
either increase the delay to >=250ms or invoke showBanner after the dialog's
close animation completes. Locate the setTimeout(...) call that wraps showBanner
in page-satisfaction-banner.tsx (and the showBanner(...) invocation) and replace
the 150ms delay with ~250ms (or better: move the showBanner call into the dialog
close/afterClose callback or promise from ReusableDialog so it runs only after
the dialog's exit animation finishes). Ensure you update the call site to use
the dialog's provided onClose/afterClose hook or callback rather than a
hardcoded short timeout.
In `@src/vertex/components/layout/layout.tsx`:
- Around line 140-144: The mobile bottom padding was moved from the outer <main>
to the inner content wrapper, so Footer and PageSatisfactionBanner now sit
outside that padded area and may overlap fixed bottom UI; update the layout so
mobile bottom spacing applies to elements that include
Footer/PageSatisfactionBanner — either restore the mobile padding (pb-20) to the
outer container that uses isSecondarySidebarCollapsed (the element with the long
className including transition-[margin-left]) or add safe bottom spacing (e.g.,
safe-area-inset-bottom or pb-20 on the wrapper that renders
Footer/PageSatisfactionBanner) so those components don’t overlap persistent
bottom navigation on small screens. Ensure the change targets the className on
the outer flex-1 container or the wrapper that actually contains
Footer/PageSatisfactionBanner.
---
Nitpick comments:
In `@src/vertex/components/features/feedback/page-satisfaction-banner.tsx`:
- Around line 258-272: Replace the two FeedbackModal renderings with a single
FeedbackModal driven by modalType: use isOpen based on modalType (e.g.,
modalType !== null), pass type using modalType (coerced or defaulted as needed),
keep onClose={() => setModalType(null)}, onSubmit={handleSubmitFeedback} and
isSubmitting={isSubmitting}; this collapses the duplicated JSX while preserving
behavior and opens the correct modal content based on modalType.
- Around line 184-187: The user-agent is being truncated via
navigator.userAgent.slice(0, 80) when building the browser field—add a short
inline comment (or extract the number into a named constant like
UA_TRUNCATE_LENGTH) that documents why 80 chars was chosen and the tradeoffs
(what is preserved, what may be lost), and consider making the length
configurable or removing truncation if full UA is required; update the code
around the browser assignment (the browser field and the
navigator.userAgent.slice(0, 80) usage) to include this explanatory
comment/constant so future readers understand the rationale.
- Line 202: Remove the unnecessary blank line in the function around the
conditional that currently has an empty line immediately before the `return
true` statement in page-satisfaction-banner.tsx; locate the function or method
(e.g., the helper that returns a boolean in this component) and delete the stray
blank line so `return true` follows the previous statement directly for
consistent formatting.
- Line 182: Remove the unnecessary fallback to window.location.pathname in the
PageSatisfactionBanner component: where the payload sets page: pathname ||
window.location.pathname, replace it to use the pathname value directly (page:
pathname) since usePathname() from next/navigation always supplies a string in
the App Router; update any related references in the same component to rely on
pathname only and remove the window global usage.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5536b18e-935a-403a-9388-7779250cf1e1
📒 Files selected for processing (5)
src/vertex/app/changelog.mdsrc/vertex/app/providers.tsxsrc/vertex/components/features/feedback/page-satisfaction-banner.tsxsrc/vertex/components/layout/layout.tsxsrc/vertex/context/banner-context.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
New Features
Bug Fixes