Enhance feedback details and authentication handling#3547
Merged
Conversation
- Added app and screenshot_url fields to FeedbackSubmission interface. - Updated waitForNextAuthSession to validate expected access token. - Improved OAuth token handling in AuthProvider for better session management. - Refactored FeedbackDetailsContent to display additional feedback metadata and improve UI components.
Contributor
|
Warning Review limit reached
More reviews will be available in 26 minutes and 8 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds optional ChangesFeedback System Enhancement
Sequence Diagram(s)sequenceDiagram
participant User
participant FeedbackLauncher
participant Annotator as ScreenshotAnnotator
participant Cloudinary
participant FeedbackAPI as feedbackService.submitFeedback
User->>FeedbackLauncher: open dialog, capture or upload image
FeedbackLauncher->>Annotator: draw/highlight rectangles (optional)
Annotator-->>FeedbackLauncher: flattened image (data URL / File)
FeedbackLauncher->>Cloudinary: upload annotated image (POST)
Cloudinary-->>FeedbackLauncher: returns screenshot_url
FeedbackLauncher->>FeedbackAPI: submitFeedback(payload with screenshot_url)
FeedbackAPI-->>FeedbackLauncher: confirmation
FeedbackLauncher-->>User: show success/failure toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
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 |
Contributor
|
New azure analytics_platform changes available for preview here |
![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: 2
🤖 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/platform/package.json`:
- Line 6: The prebuild script still duplicates the inline cleanup command
instead of reusing the existing "clean:next" script; update the "prebuild"
script to call the "clean:next" script (e.g., invoke it via an npm script run)
rather than repeating the node:fs rmSync command so the cleanup logic is
centralized in the "clean:next" script; locate "prebuild" and replace the inline
removal invocation with a call to "clean:next".
In `@src/platform/src/app/`(dashboard)/system/feedback/[feedbackId]/page.tsx:
- Around line 132-142: The metadata mapping drops the original metadata key and
only keeps the formatted label, which can lead to duplicate React keys when
formatted labels collide; change the mapping that builds metadataEntries to
preserve the original key (e.g., store tuples like [key, formatKeyLabel(key),
String(value)] or [formatKeyLabel(key), String(value), key]) and then update any
destructuring/consumers (the code that currently uses formatKeyLabel(...) and
String(value) — referenced as formatKeyLabel and metadataEntries and the
subsequent map that renders rows) to use the preserved original key as the
unique React key while still displaying the formatted label.
🪄 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: 07597d6b-dba4-469a-a772-c4cb4ad1b244
📒 Files selected for processing (4)
src/platform/package.jsonsrc/platform/src/app/(dashboard)/system/feedback/[feedbackId]/page.tsxsrc/platform/src/shared/providers/auth-provider.tsxsrc/platform/src/shared/types/api.ts
Contributor
|
New azure analytics_platform changes available for preview here |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/platform/src/modules/feedback/components/FeedbackLauncher.tsx (2)
829-833: ⚡ Quick winConsider logging the upload error for debugging purposes.
The catch block gracefully degrades by showing a warning and continuing submission—good UX. However, the error itself is discarded. Logging it (even just to console in development or a monitoring service in production) would help diagnose upload failures.
- } catch { + } catch (uploadError) { + console.warn('Screenshot upload failed:', uploadError); toast.warning( 'Screenshot upload failed', 'Your feedback will still be submitted without the screenshot.' );🤖 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/platform/src/modules/feedback/components/FeedbackLauncher.tsx` around lines 829 - 833, The catch block in FeedbackLauncher’s screenshot upload (inside the try/catch around the upload call) swallows the error—update the catch to accept the error parameter (e.g., catch (err)) and log it for debugging; call console.error(err, 'Screenshot upload failed') or forward to the app’s monitoring/logger (e.g., monitor.captureException(err)) before showing the existing toast so failures are recorded while UX remains unchanged.
343-383: 💤 Low valueConsider adding touch event handlers for tablet users.
The annotator only handles mouse events. If tablet/touch device support matters for this admin tool, you'd need
onTouchStart,onTouchMove, andonTouchEndhandlers with coordinate extraction fromtouch.clientX/clientY. That said, if this is desktop-focused, it's fine to defer.🤖 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/platform/src/modules/feedback/components/FeedbackLauncher.tsx` around lines 343 - 383, Add touch support by implementing onTouchStart, onTouchMove and onTouchEnd handlers analogous to handleMouseDown/handleMouseMove/handleMouseUp: extract coordinates from event.touches[0] (use clientX/clientY) or create a helper getTouchPosition mirroring getPosition, call event.preventDefault(), set startRef.current, setDrawing(true), and setCurrentRect on touch start; update current rect on touch move using the same math as handleMouseMove; on touch end compute final rect, push to setRects if big enough, clear startRef/currentRect and setDrawing(false) like handleMouseUp; ensure you use the same unique symbols (handleMouseDown, handleMouseMove, handleMouseUp, startRef, setCurrentRect, setRects, setDrawing) so touch handlers integrate with the existing state logic.
🤖 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.
Nitpick comments:
In `@src/platform/src/modules/feedback/components/FeedbackLauncher.tsx`:
- Around line 829-833: The catch block in FeedbackLauncher’s screenshot upload
(inside the try/catch around the upload call) swallows the error—update the
catch to accept the error parameter (e.g., catch (err)) and log it for
debugging; call console.error(err, 'Screenshot upload failed') or forward to the
app’s monitoring/logger (e.g., monitor.captureException(err)) before showing the
existing toast so failures are recorded while UX remains unchanged.
- Around line 343-383: Add touch support by implementing onTouchStart,
onTouchMove and onTouchEnd handlers analogous to
handleMouseDown/handleMouseMove/handleMouseUp: extract coordinates from
event.touches[0] (use clientX/clientY) or create a helper getTouchPosition
mirroring getPosition, call event.preventDefault(), set startRef.current,
setDrawing(true), and setCurrentRect on touch start; update current rect on
touch move using the same math as handleMouseMove; on touch end compute final
rect, push to setRects if big enough, clear startRef/currentRect and
setDrawing(false) like handleMouseUp; ensure you use the same unique symbols
(handleMouseDown, handleMouseMove, handleMouseUp, startRef, setCurrentRect,
setRects, setDrawing) so touch handlers integrate with the existing state logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d1a1eac-cbda-463c-8ea2-4f87fdde1228
📒 Files selected for processing (4)
src/platform/package.jsonsrc/platform/src/app/(dashboard)/system/feedback/[feedbackId]/page.tsxsrc/platform/src/modules/feedback/components/FeedbackLauncher.tsxsrc/platform/src/shared/types/api.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/platform/src/shared/types/api.ts
- src/platform/package.json
- src/platform/src/app/(dashboard)/system/feedback/[feedbackId]/page.tsx
…pending group switches
Contributor
|
New azure analytics_platform changes available for preview here |
Contributor
|
New azure analytics_platform changes available for preview here |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/platform/src/shared/components/ui/group-switch-overlay.tsx (1)
76-79: ⚡ Quick winUse a small overlay delay to reduce route-switch flash.
delayMs={0}can produce noticeable flashes on quick transitions; a short delay keeps the UX smoother.💡 Suggested tweak
- delayMs={0} + delayMs={120}As per coding guidelines, "Avoid full-white loading flashes; prefer delayed overlays and subtle backdrops".
🤖 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/platform/src/shared/components/ui/group-switch-overlay.tsx` around lines 76 - 79, The LoadingOverlay currently uses delayMs={0}, which can cause visible white flashes during quick route switches; update the LoadingOverlay component usage (the instance rendering title={`Switching to ${pendingGroupSwitch.targetGroupName}`} and description) to use a small positive delay (e.g., delayMs around 150-200) so the overlay only shows for slower transitions and avoids flash—adjust the prop on the LoadingOverlay and keep the title/description as-is.
🤖 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/platform/src/shared/components/header/components/organization-selector.tsx`:
- Around line 34-36: The destinationPath construction can produce
'/org/undefined/dashboard' when selectedGroup.organizationSlug is missing;
update the logic in the code that sets destinationPath (currently using
isDefaultAirQoGroup(selectedGroup) and selectedGroup.organizationSlug) to first
check that selectedGroup.organizationSlug is truthy (and optionally non-empty)
before building `/org/${selectedGroup.organizationSlug}/dashboard`, and fall
back to '/user/home' (or another safe route) when the slug is absent or falsy;
reference the variables destinationPath, isDefaultAirQoGroup, and
selectedGroup.organizationSlug when making this guard and consider encoding the
slug if present.
In `@src/platform/src/shared/components/ui/group-switch-overlay.tsx`:
- Around line 51-54: The code returns early when startedAt is invalid and leaves
pendingGroupSwitch set, which can leave the overlay stuck; update the
invalid-startedAt branch to clear the pending state before returning—e.g., call
the existing clearPendingGroupSwitch() helper (or the setter like
setPendingGroupSwitch(null)/undefined) right before the return in
group-switch-overlay.tsx so pendingGroupSwitch is reset when
Number.isFinite(startedAt) is false.
In `@src/platform/src/shared/components/ui/toast.tsx`:
- Around line 257-261: The toast container div (role="alert",
aria-live="polite") uses a fixed min-w-[340px] which can overflow narrow
viewports; change the Tailwind class to make the minimum width apply only at sm
and up (e.g., replace min-w-[340px] with sm:min-w-[340px] or use min-w-0 +
sm:min-w-[340px]) on the element that has className="w-full min-w-[340px]
max-w-[560px]" so mobile screens can shrink the toast while preserving desktop
sizing.
- Around line 255-260: The wrapper rendered in sonnerToast.custom (the JSX
returned for toastId) currently uses role="alert" and aria-live="polite"; change
this to choose role and aria-live based on the toast type so only errors use
assertive alerts. Inside the custom renderer (the id => (...) function) read the
toast type (e.g., from the toast payload or sonner API) and set role={toastType
=== 'error' ? 'alert' : 'status'} and aria-live={toastType === 'error' ?
'assertive' : 'polite'} so success/info/warning toasts use a polite status
region while errors remain assertive alerts.
---
Nitpick comments:
In `@src/platform/src/shared/components/ui/group-switch-overlay.tsx`:
- Around line 76-79: The LoadingOverlay currently uses delayMs={0}, which can
cause visible white flashes during quick route switches; update the
LoadingOverlay component usage (the instance rendering title={`Switching to
${pendingGroupSwitch.targetGroupName}`} and description) to use a small positive
delay (e.g., delayMs around 150-200) so the overlay only shows for slower
transitions and avoids flash—adjust the prop on the LoadingOverlay and keep the
title/description as-is.
🪄 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: 10e422c1-aef8-4b72-9188-fe49aa8b2b5a
📒 Files selected for processing (9)
src/platform/package.jsonsrc/platform/src/shared/components/header/components/organization-selector.tsxsrc/platform/src/shared/components/ui/group-switch-overlay.tsxsrc/platform/src/shared/components/ui/loading-overlay.tsxsrc/platform/src/shared/components/ui/toast.tsxsrc/platform/src/shared/providers/UserDataFetcher.tsxsrc/platform/src/shared/providers/auth-provider.tsxsrc/platform/src/shared/store/selectors.tssrc/platform/src/shared/store/userSlice.ts
…and update Toast component accessibility roles
Contributor
|
New azure analytics_platform changes available for preview here |
Contributor
|
New azure analytics_platform changes available for preview here |
Member
|
Feedback unrelated to the changes in the PR, when I use google sign on the preview, I am taken to staging and when I return to the deploy review link, it keeps taking me back to staging analytics so I cannot test with the deploy preview @OchiengPaul442 |
Member
|
Is there already a way of sorting feedback by application ie sorting to view only vertex feedback or analytics feedback alone @OchiengPaul442 ? |
Contributor
Author
|
Still investigating the cause of redirect on PR previews as for the second feedback thats noted, thanks. @Codebmk |
Contributor
|
New azure analytics_platform changes available for preview here |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/platform/src/app/(dashboard)/system/feedback/page.tsx (1)
69-74: ⚡ Quick winAvoid casing-mismatch risk only if backend varies
feedback.appcasingThe filter uses strict equality (
feedback.app === appFilter) andAPP_OPTIONSmixes casing ('Analytics'vs'beacon'/'vertex'). However, the only submission path found setsapp: FEEDBACK_APP_NAME, andFEEDBACK_APP_NAMEis hard-coded to'Analytics', with no othersubmitFeedbackcall sites settingapptobeacon/vertex. So the current client submission flow shouldn’t hit a casing-based miss forAnalytics.If the backend can persist
feedback.appwith different casing (e.g., from other producers), normalizing for comparison is still a safe hardening—without changing the option values.🤖 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/platform/src/app/`(dashboard)/system/feedback/page.tsx around lines 69 - 74, The app filter risks casing mismatches because APP_OPTIONS contains mixed-case labels while feedback.app may vary; update the filter that currently uses strict equality (feedback.app === appFilter) to compare case-insensitively by normalizing both sides (e.g., (feedback.app || '').toLowerCase() === (appFilter || '').toLowerCase()), leaving APP_OPTIONS values unchanged; reference symbols: APP_OPTIONS, feedback.app, appFilter, FEEDBACK_APP_NAME, and any submitFeedback call sites to ensure undefined/null safety when normalizing.
🤖 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.
Nitpick comments:
In `@src/platform/src/app/`(dashboard)/system/feedback/page.tsx:
- Around line 69-74: The app filter risks casing mismatches because APP_OPTIONS
contains mixed-case labels while feedback.app may vary; update the filter that
currently uses strict equality (feedback.app === appFilter) to compare
case-insensitively by normalizing both sides (e.g., (feedback.app ||
'').toLowerCase() === (appFilter || '').toLowerCase()), leaving APP_OPTIONS
values unchanged; reference symbols: APP_OPTIONS, feedback.app, appFilter,
FEEDBACK_APP_NAME, and any submitFeedback call sites to ensure undefined/null
safety when normalizing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c5c512a-df23-4e12-849a-6c6d31f3ee9c
📒 Files selected for processing (4)
src/platform/src/app/(dashboard)/system/feedback/page.tsxsrc/platform/src/shared/components/header/components/organization-selector.tsxsrc/platform/src/shared/components/ui/group-switch-overlay.tsxsrc/platform/src/shared/components/ui/toast.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/platform/src/shared/components/header/components/organization-selector.tsx
- src/platform/src/shared/components/ui/group-switch-overlay.tsx
Contributor
|
New azure analytics_platform changes available for preview here |
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
3 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.
Status of maturity (all need to be checked before merging):
Screenshots (optional)
Summary by CodeRabbit
New Features
Bug Fixes
Chores