Update Site Management Module to use InfoBanners#3599
Merged
Baalmart merged 11 commits intoJun 10, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes Site Management UI feedback (banners/clipboard), refactors site hooks to use callback lifecycles, adds a severity-aware metadata refresh wrapper, and introduces version-aware API routing plus OAuth signed-out flag and bootstrap guards. ChangesSite Management Banner & Clipboard Migration
API Routing & OAuth Safety Fixes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (Client)
participant SocialAuth as SocialAuthSection
participant AuthProv as AuthProvider
participant OAuthSession as oauth-session helpers
participant Backend as Backend OAuth
Browser->>SocialAuth: user clicks provider
SocialAuth->>OAuthSession: clearBackendOAuthSignedOutFlag()
SocialAuth->>Browser: redirect to Backend OAuth (with prompt=select_account for Google)
Browser->>AuthProv: returns with ?oauth_token
AuthProv->>OAuthSession: consumeOAuthTokenHandoffFromUrl()
AuthProv->>OAuthSession: clearBackendOAuthSignedOutFlag() before sign-in
AuthProv->>AuthProv: if no token -> shouldSkipBackendOAuthBootstrap()? -> skip bootstrap if true
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 vertex 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.
🧹 Nitpick comments (3)
src/vertex/core/hooks/useSites.ts (1)
195-224: 💤 Low valueConsider passing response data to the onSuccess callback.
The
useUpdateSiteDetailshook currently provides anonSuccesscallback with no parameters (line 196), even though the mutation receives response data. The callback could receive the update response (which includesmessageand updatedSitedata per context snippet 2) to enable richer UI feedback, such as displaying the backend's confirmation message.♻️ Suggested enhancement
interface UseUpdateSiteDetailsOptions { - onSuccess?: () => void; + onSuccess?: (data: unknown) => void; onError?: (error: AxiosError) => void; }onSuccess: (_data, { siteId }) => { queryClient.invalidateQueries({ queryKey: ["site-details", siteId] }); queryClient.invalidateQueries({ queryKey: ["sites"] }); - options?.onSuccess?.(); + options?.onSuccess?.(_data); },🤖 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/core/hooks/useSites.ts` around lines 195 - 224, The hook currently declares UseUpdateSiteDetailsOptions.onSuccess as a no-arg callback and then ignores the mutation response in useUpdateSiteDetails; change the interface to accept the mutation response (e.g. onSuccess?: (data: unknown) => void or a more specific response type), and forward the mutation result to it by passing the first onSuccess parameter from the useMutation onSuccess handler (replace options?.onSuccess?.() with options?.onSuccess?.(data)). Update any related types for clarity (UseUpdateSiteDetailsOptions and the useMutation generic if you want a concrete response type) so callers can consume the backend message and updated Site data.src/vertex/app/(authenticated)/sites/[id]/page.tsx (1)
37-51: ⚡ Quick winExtract duplicated refresh metadata callback logic.
Lines 37-51 are identical to the admin page implementation (admin/sites/[id]/page.tsx:35-49). Both pages perform the same message inspection and severity determination. Extract this logic into a shared custom hook like
useRefreshMetadataWithBannerto improve maintainability.♻️ Suggested refactor
Create a new hook file:
// src/vertex/core/hooks/useRefreshMetadataWithBanner.ts export const useRefreshMetadataWithBanner = () => { const { showBanner } = useBanner(); const handleSuccess = (data: SiteRefreshResponse) => { const msg = (data.message ?? "").toLowerCase(); if (msg.includes("partially refreshed")) { showBanner({ severity: 'warning', message: data.message, scoped: false }); } else if (msg.includes("already complete")) { showBanner({ severity: 'info', message: 'Site metadata is already up to date.', scoped: false }); } else { showBanner({ severity: 'success', message: 'Site metadata refreshed successfully.', scoped: false }); } }; const handleError = (error: AxiosError) => { showBanner({ severity: 'error', message: `Refresh Failed: ${getApiErrorMessage(error)}`, scoped: false }); }; return useRefreshSiteMetadata({ onSuccess: handleSuccess, onError: handleError }); };Then both pages can use:
const { mutate: refreshMetadata, isPending: isRefreshing } = useRefreshMetadataWithBanner();🤖 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/app/`(authenticated)/sites/[id]/page.tsx around lines 37 - 51, Extract the duplicated success/error handling for site refresh into a shared hook named useRefreshMetadataWithBanner: move the onSuccess logic that inspects (data.message ?? "").toLowerCase() and calls showBanner with severity 'warning'/'info'/'success' and the onError logic that calls showBanner with `Refresh Failed: ${getApiErrorMessage(error)}` into the hook, type the success param as SiteRefreshResponse and the error as AxiosError, and have the hook return the result of calling useRefreshSiteMetadata({ onSuccess: handleSuccess, onError: handleError }) so both page components can replace their local mutate/isPending usage with `const { mutate: refreshMetadata, isPending: isRefreshing } = useRefreshMetadataWithBanner();`.src/vertex/app/(authenticated)/admin/sites/[id]/page.tsx (1)
35-49: 💤 Low valueWarning banner displays raw backend message.
At line 39, when the refresh is partial,
data.messageis displayed directly to the user. Backend messages may contain technical jargon (e.g., "grids: no matching grids for site coordinates" from Image 7 in PR description). Consider sanitizing or wrapping technical details for clarity.💬 Suggested improvement
if (msg.includes("partially refreshed")) { - showBanner({ severity: 'warning', message: data.message, scoped: false }); + showBanner({ + severity: 'warning', + message: `Site partially refreshed — some steps were unsuccessful: ${data.message.split(':').slice(1).join(':').trim()}`, + scoped: false + }); }Alternatively, display a generic warning and log the technical details.
🤖 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/app/`(authenticated)/admin/sites/[id]/page.tsx around lines 35 - 49, When handling the partial-refresh case in the useRefreshSiteMetadata callback (mutate alias refreshMetadata), do not display raw backend text from data.message directly via showBanner; instead show a user-friendly warning like "Site metadata partially refreshed — some items could not be updated." and send the full technical text (data.message) to logs/telemetry (e.g., console.error or your error-tracking utility) for diagnostics. Update the onSuccess handler in the useRefreshSiteMetadata usage to replace the direct showBanner(data.message) path with the generic message and ensure data.message is recorded to logs/telemetry for developers.
🤖 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/vertex/app/`(authenticated)/admin/sites/[id]/page.tsx:
- Around line 35-49: When handling the partial-refresh case in the
useRefreshSiteMetadata callback (mutate alias refreshMetadata), do not display
raw backend text from data.message directly via showBanner; instead show a
user-friendly warning like "Site metadata partially refreshed — some items could
not be updated." and send the full technical text (data.message) to
logs/telemetry (e.g., console.error or your error-tracking utility) for
diagnostics. Update the onSuccess handler in the useRefreshSiteMetadata usage to
replace the direct showBanner(data.message) path with the generic message and
ensure data.message is recorded to logs/telemetry for developers.
In `@src/vertex/app/`(authenticated)/sites/[id]/page.tsx:
- Around line 37-51: Extract the duplicated success/error handling for site
refresh into a shared hook named useRefreshMetadataWithBanner: move the
onSuccess logic that inspects (data.message ?? "").toLowerCase() and calls
showBanner with severity 'warning'/'info'/'success' and the onError logic that
calls showBanner with `Refresh Failed: ${getApiErrorMessage(error)}` into the
hook, type the success param as SiteRefreshResponse and the error as AxiosError,
and have the hook return the result of calling useRefreshSiteMetadata({
onSuccess: handleSuccess, onError: handleError }) so both page components can
replace their local mutate/isPending usage with `const { mutate:
refreshMetadata, isPending: isRefreshing } = useRefreshMetadataWithBanner();`.
In `@src/vertex/core/hooks/useSites.ts`:
- Around line 195-224: The hook currently declares
UseUpdateSiteDetailsOptions.onSuccess as a no-arg callback and then ignores the
mutation response in useUpdateSiteDetails; change the interface to accept the
mutation response (e.g. onSuccess?: (data: unknown) => void or a more specific
response type), and forward the mutation result to it by passing the first
onSuccess parameter from the useMutation onSuccess handler (replace
options?.onSuccess?.() with options?.onSuccess?.(data)). Update any related
types for clarity (UseUpdateSiteDetailsOptions and the useMutation generic if
you want a concrete response type) so callers can consume the backend message
and updated Site data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c129bc5d-81be-4073-94b5-a350b7d48277
📒 Files selected for processing (8)
src/vertex/app/(authenticated)/admin/sites/[id]/page.tsxsrc/vertex/app/(authenticated)/sites/[id]/page.tsxsrc/vertex/app/changelog.mdsrc/vertex/components/features/sites/create-site-form.tsxsrc/vertex/components/features/sites/edit-site-details-dialog.tsxsrc/vertex/components/features/sites/site-information-card.tsxsrc/vertex/components/features/sites/site-measurements-api-card.tsxsrc/vertex/core/hooks/useSites.ts
Contributor
|
New azure vertex changes available for preview here |
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates Site Management user notifications from ReusableToast / Sonner toast to the centralized banner + clipboard notification system, while decoupling React Query mutation hooks from UI-side notification responsibilities.
Changes:
- Refactored key site mutations in
useSites.tsto accept optionalonSuccess/onErrorcallbacks instead of triggering toasts internally. - Updated Site Management dialogs/cards/pages to use
useBanner,useBannerWithDelay, anduseClipboardfor scoped vs global notifications. - Added a shared
useRefreshMetadataWithBannerhook to centralize severity-aware refresh messaging.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vertex/core/hooks/useSites.ts | Removes toast dependency from mutations and delegates success/error handling to callers via options callbacks. |
| src/vertex/core/hooks/useRefreshMetadataWithBanner.ts | New helper hook that wraps metadata refresh with global severity-aware banners. |
| src/vertex/components/features/sites/create-site-form.tsx | Uses banner hooks for scoped errors and delayed global success after create. |
| src/vertex/components/features/sites/edit-site-details-dialog.tsx | Replaces Sonner toasts with scoped banners and delayed global success on mutation completion. |
| src/vertex/components/features/sites/site-information-card.tsx | Uses useClipboard for copy-to-clipboard feedback via global banners. |
| src/vertex/components/features/sites/site-measurements-api-card.tsx | Uses useClipboard for copy-to-clipboard feedback (but currently missing client directive). |
| src/vertex/app/(authenticated)/admin/sites/[id]/page.tsx | Switches refresh metadata action to the shared banner-wrapped hook. |
| src/vertex/app/(authenticated)/sites/[id]/page.tsx | Switches refresh metadata action to the shared banner-wrapped hook. |
| src/vertex/app/changelog.md | Adds a release note entry for the banner migration (but section counts are currently inconsistent). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
17
to
18
| import { AxiosError } from "axios"; | ||
| import { getApiErrorMessage } from "../utils/getApiErrorMessage"; |
Comment on lines
1
to
+5
| import { Card } from "@/components/ui/card"; | ||
| import { Button } from "@/components/ui/button"; | ||
| import { Copy } from "lucide-react"; | ||
| import React from "react"; | ||
| import ReusableToast from "@/components/shared/toast/ReusableToast"; | ||
| import { useClipboard } from "@/core/hooks/useClipboard"; |
| Migrated all user-facing notifications in the Site Management module from `ReusableToast` and Sonner `toast` to the centralized `useBanner` / `useBannerWithDelay` / `useClipboard` system. | ||
|
|
||
| <details> | ||
| <summary><strong>Changes (5)</strong></summary> |
| </details> | ||
|
|
||
| <details> | ||
| <summary><strong>Files Updated (7)</strong></summary> |
Comment on lines
+10
to
+19
| const handleSuccess = (data: SiteRefreshResponse) => { | ||
| const msg = (data.message ?? "").toLowerCase(); | ||
| if (msg.includes("partially refreshed")) { | ||
| showBanner({ severity: "warning", message: data.message, scoped: false }); | ||
| } else if (msg.includes("already complete")) { | ||
| showBanner({ severity: "info", message: "Site metadata is already up to date.", scoped: false }); | ||
| } else { | ||
| showBanner({ severity: "success", message: "Site metadata refreshed successfully.", scoped: false }); | ||
| } | ||
| }; |
Comment on lines
+137
to
+140
| interface UseApproximateCoordinatesOptions { | ||
| onSuccess?: (data: ApproximateCoordinatesResponse) => void; | ||
| onError?: (error: AxiosError) => void; | ||
| } |
Comment on lines
+211
to
218
| onSuccess: (_data, { siteId }) => { | ||
| queryClient.invalidateQueries({ queryKey: ["site-details", siteId] }); | ||
| queryClient.invalidateQueries({ queryKey: ["sites"] }); | ||
| options?.onSuccess?.(); | ||
| }, | ||
| onError: (error) => { | ||
| ReusableToast({ | ||
| message: `Failed to update site: ${getApiErrorMessage(error)}`, | ||
| type: "ERROR", | ||
| }); | ||
| options?.onError?.(error); | ||
| }, |
Contributor
|
New azure vertex changes available for preview here |
Comment on lines
+155
to
160
| onSuccess: (data) => { | ||
| try { options?.onSuccess?.(data); } catch (e) { console.error(e); } | ||
| }, | ||
| onError: (error) => { | ||
| ReusableToast({ | ||
| message: `Unable to get approximate coordinates: ${getApiErrorMessage(error)}`, | ||
| type: "ERROR", | ||
| }); | ||
| try { options?.onError?.(error); } catch (e) { console.error(e); } | ||
| }, |
Comment on lines
+191
to
+194
| interface UseUpdateSiteDetailsOptions { | ||
| onSuccess?: () => void; | ||
| onError?: (error: AxiosError<ErrorResponse>) => void; | ||
| } |
Comment on lines
+58
to
+62
| showBannerWithDelay({ severity: 'success', message: 'Site details updated successfully', scoped: false }); | ||
| }, | ||
| onError: (error) => { | ||
| showBanner({ severity: 'error', message: `Failed to update site: ${getApiErrorMessage(error)}`, scoped: true }); | ||
| }, |
Comment on lines
+78
to
+87
| showBanner({ severity: 'error', message: `Unable to get approximate coordinates: ${getApiErrorMessage(error)}`, scoped: true }); | ||
| }, | ||
| }); | ||
| const { mutate: createSite, isPending: isCreating } = useCreateSite({ | ||
| onSuccess: (_data, variables) => { | ||
| showBannerWithDelay({ severity: 'success', message: `Site '${variables.name}' created successfully`, scoped: false }); | ||
| }, | ||
| onError: (error) => { | ||
| showBanner({ severity: 'error', message: `Failed to create site: ${getApiErrorMessage(error)}`, scoped: true }); | ||
| }, |
Contributor
|
New azure vertex changes available for preview here |
Comment on lines
+155
to
160
| onSuccess: (data) => { | ||
| try { options?.onSuccess?.(data); } catch (e) { console.error(e); } | ||
| }, | ||
| onError: (error) => { | ||
| ReusableToast({ | ||
| message: `Unable to get approximate coordinates: ${getApiErrorMessage(error)}`, | ||
| type: "ERROR", | ||
| }); | ||
| try { options?.onError?.(error); } catch (e) { console.error(e); } | ||
| }, |
Comment on lines
216
to
218
| onError: (error) => { | ||
| ReusableToast({ | ||
| message: `Failed to update site: ${getApiErrorMessage(error)}`, | ||
| type: "ERROR", | ||
| }); | ||
| try { options?.onError?.(error); } catch (e) { console.error(e); } | ||
| }, |
Comment on lines
+7
to
+26
| export const useRefreshMetadataWithBanner = () => { | ||
| const { showBanner } = useBanner(); | ||
|
|
||
| const handleSuccess = (data: SiteRefreshResponse) => { | ||
| const msg = (data.message ?? "").toLowerCase(); | ||
| if (msg.includes("partially refreshed")) { | ||
| showBanner({ severity: "warning", message: data.message ?? "Site metadata partially refreshed.", scoped: false }); | ||
| } else if (msg.includes("already complete")) { | ||
| showBanner({ severity: "info", message: "Site metadata is already up to date.", scoped: false }); | ||
| } else { | ||
| showBanner({ severity: "success", message: "Site metadata refreshed successfully.", scoped: false }); | ||
| } | ||
| }; | ||
|
|
||
| const handleError = (error: AxiosError) => { | ||
| showBanner({ severity: "error", message: `Refresh Failed: ${getApiErrorMessage(error)}`, scoped: false }); | ||
| }; | ||
|
|
||
| return useRefreshSiteMetadata({ onSuccess: handleSuccess, onError: handleError }); | ||
| }; |
Comment on lines
26
to
29
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="hover:bg-transparent" |
Comment on lines
+30
to
33
| onClick={() => handleCopy(`https://api.airqo.net/api/v2/devices/measurements/sites/${siteId}/recent?token=YOUR_TOKEN`)} | ||
| > | ||
| <Copy className="w-4 h-4" /> | ||
| </Button> |
Contributor
|
New azure vertex changes available for preview here |
…o-use-InfoBanners
Comment on lines
+1
to
+3
| import { useCallback } from "react"; | ||
| import { useRefreshSiteMetadata } from "@/core/hooks/useSites"; | ||
| import { useBanner } from "@/context/banner-context"; |
Contributor
Author
There was a problem hiding this comment.
@copilot Hooks can only be called from client components which use use client.
Comment on lines
+191
to
+194
| interface UseUpdateSiteDetailsOptions { | ||
| onSuccess?: () => void; | ||
| onError?: (error: AxiosError<ErrorResponse>) => void; | ||
| } |
Comment on lines
+211
to
215
| onSuccess: (_data, { siteId }) => { | ||
| queryClient.invalidateQueries({ queryKey: ["site-details", siteId] }); | ||
| queryClient.invalidateQueries({ queryKey: ["sites"] }); | ||
| options?.onSuccess?.(); | ||
| }, |
Comment on lines
+267
to
+270
| interface UseRefreshSiteMetadataOptions { | ||
| onSuccess?: (data: SiteRefreshResponse) => void; | ||
| onError?: (error: AxiosError<ErrorResponse>) => void; | ||
| } |
Comment on lines
277
to
282
| onSuccess: (data, siteId) => { | ||
| // Update the cache with the newly enriched site data | ||
| queryClient.setQueryData(["site-details", siteId], data.site); | ||
| queryClient.invalidateQueries({ queryKey: ["sites"] }); | ||
| queryClient.invalidateQueries({ queryKey: ["site-details", siteId] }); | ||
|
|
||
| const msg = (data.message ?? "").toLowerCase(); | ||
| if (msg.includes("partially refreshed")) { | ||
| ReusableToast({ | ||
| message: data.message, | ||
| type: "WARNING", | ||
| }); | ||
| } else if (msg.includes("already complete")) { | ||
| ReusableToast({ | ||
| message: "Site metadata is already up to date.", | ||
| type: "INFO", | ||
| }); | ||
| } else { | ||
| ReusableToast({ | ||
| message: "Site metadata refreshed successfully.", | ||
| type: "SUCCESS", | ||
| }); | ||
| } | ||
| options?.onSuccess?.(data); | ||
| }, |
Comment on lines
+22
to
+24
| const handleError = useCallback((error: AxiosError) => { | ||
| showBanner({ severity: "error", message: `Refresh Failed: ${getApiErrorMessage(error)}`, scoped: false }); | ||
| }, [showBanner]); |
Contributor
|
New azure docs changes available for preview here |
Contributor
|
New azure vertex changes available for preview here |
Contributor
|
New azure analytics_platform changes available for preview here |
| onError?: (error: AxiosError<ErrorResponse>) => void; | ||
| } | ||
|
|
||
| export const useApproximateCoordinates = (options?: UseApproximateCoordinatesOptions) => { |
Comment on lines
+155
to
160
| onSuccess: (data) => { | ||
| options?.onSuccess?.(data); | ||
| }, | ||
| onError: (error) => { | ||
| ReusableToast({ | ||
| message: `Unable to get approximate coordinates: ${getApiErrorMessage(error)}`, | ||
| type: "ERROR", | ||
| }); | ||
| options?.onError?.(error); | ||
| }, |
Comment on lines
+8
to
+13
| interface ErrorResponse { | ||
| message: string; | ||
| errors?: { | ||
| message: string; | ||
| }; | ||
| } |
Comment on lines
+18
to
+27
| const handleSuccess = useCallback((data: SiteRefreshResponse) => { | ||
| const msg = (data.message ?? "").toLowerCase(); | ||
| if (msg.includes("partially refreshed")) { | ||
| showBanner({ severity: "warning", message: data.message ?? "Site metadata partially refreshed.", scoped: false }); | ||
| } else if (msg.includes("already complete")) { | ||
| showBanner({ severity: "info", message: "Site metadata is already up to date.", scoped: false }); | ||
| } else { | ||
| showBanner({ severity: "success", message: "Site metadata refreshed successfully.", scoped: false }); | ||
| } | ||
| }, [showBanner]); |
| showBanner({ severity: "error", message: `Refresh Failed: ${getApiErrorMessage(error)}`, scoped: false }); | ||
| }, [showBanner]); | ||
|
|
||
| return useRefreshSiteMetadata({ onSuccess: handleSuccess, onError: handleError }); |
Contributor
|
New azure docs changes available for preview here |
Contributor
|
New azure analytics_platform changes available for preview here |
Contributor
|
New azure vertex changes available for preview here |
…o-use-InfoBanners
Contributor
|
New azure docs changes available for preview here |
Contributor
|
New azure analytics_platform changes available for preview here |
Contributor
|
New azure vertex changes available for preview here |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vertex/core/auth/oauth-session.ts (1)
156-166:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate the callback
successvalue before treating it as the OAuth provider.
provider || getLastUsedOAuthProvider()only falls back when the query param is empty. Any non-empty unsupportedsuccessvalue from the callback URL will still be forwarded into the handoff flow, so a malformed callback breaks sign-in instead of recovering with the stored provider.Suggested fix
- const provider = new URLSearchParams(window.location.search) + const rawProvider = new URLSearchParams(window.location.search) .get(OAUTH_SUCCESS_PROVIDER_KEY) ?.trim() .toLowerCase(); + const provider = isSupportedSocialAuthProvider(rawProvider) + ? rawProvider + : getLastUsedOAuthProvider(); const callbackUrl = hashParams.get('callbackUrl') || new URLSearchParams(window.location.search).get('callbackUrl'); return { token, - provider: provider || getLastUsedOAuthProvider(), + provider, callbackUrl: callbackUrl || null, };🤖 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/core/auth/oauth-session.ts` around lines 156 - 166, The code reads an unvalidated provider from URLSearchParams into the provider constant (via OAUTH_SUCCESS_PROVIDER_KEY) and then uses provider || getLastUsedOAuthProvider(), which still allows unsupported non-empty values to propagate; validate the provider against the app's allowed OAuth providers (e.g., a whitelist/enum of supported provider names) before using it — if the parsed provider is missing or not in the supported list, fall back to getLastUsedOAuthProvider(); update the logic around the provider constant and return object in oauth-session.ts (the provider variable, the OAUTH_SUCCESS_PROVIDER_KEY usage, and getLastUsedOAuthProvider()) so only known providers are forwarded into the handoff flow and malformed values recover to the stored provider.
🧹 Nitpick comments (2)
src/vertex/lib/api-routing.ts (2)
53-69: ⚡ Quick winError message could list all accepted environment variables.
The function checks four environment variables but the error only mentions two. Consider updating for clarity:
- 'API base URL is not defined. Set NEXT_PUBLIC_API_URL or API_BASE_URL in environment variables.' + 'API base URL is not defined. Set API_BASE_URL, NEXT_PUBLIC_API_BASE_URL, NEXT_PUBLIC_API_URL, or NEXT_PUBLIC_BASE_URL in environment variables.'🤖 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/lib/api-routing.ts` around lines 53 - 69, The error thrown in resolveApiOrigin currently mentions only NEXT_PUBLIC_API_URL and API_BASE_URL but the function actually checks API_BASE_URL, NEXT_PUBLIC_API_BASE_URL, NEXT_PUBLIC_API_URL, and NEXT_PUBLIC_BASE_URL; update the Error message in resolveApiOrigin (which calls stripApiSuffix) to list all four accepted environment variables so users see every valid option when the normalized origin is missing.
11-27: 💤 Low valueConsider handling deeper path suffixes.
The regex patterns handle common cases (
/api/v2,/api/v2/{segment},/api), but if a base URL contains a deeper path like/api/v2/nested/path, it won't be stripped. This is likely fine for typical deployments, but worth noting if you expect varied configurations.🤖 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/lib/api-routing.ts` around lines 11 - 27, The stripApiSuffix function currently only strips /api, /api/vN, or /api/vN/{segment} but misses deeper paths; update stripApiSuffix to remove any trailing "/api" optionally followed by "/v<digits>" and any further path segments in one operation (i.e., use trimmedBaseUrl.replace(...) with a regex that matches "/api" optionally followed by "/v" + digits and any following path components) so it returns the base host for inputs like "/api/v2/nested/path"; keep the initial trimming of whitespace and trailing slashes and ensure the replacement is case-insensitive.
🤖 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.
Outside diff comments:
In `@src/vertex/core/auth/oauth-session.ts`:
- Around line 156-166: The code reads an unvalidated provider from
URLSearchParams into the provider constant (via OAUTH_SUCCESS_PROVIDER_KEY) and
then uses provider || getLastUsedOAuthProvider(), which still allows unsupported
non-empty values to propagate; validate the provider against the app's allowed
OAuth providers (e.g., a whitelist/enum of supported provider names) before
using it — if the parsed provider is missing or not in the supported list, fall
back to getLastUsedOAuthProvider(); update the logic around the provider
constant and return object in oauth-session.ts (the provider variable, the
OAUTH_SUCCESS_PROVIDER_KEY usage, and getLastUsedOAuthProvider()) so only known
providers are forwarded into the handoff flow and malformed values recover to
the stored provider.
---
Nitpick comments:
In `@src/vertex/lib/api-routing.ts`:
- Around line 53-69: The error thrown in resolveApiOrigin currently mentions
only NEXT_PUBLIC_API_URL and API_BASE_URL but the function actually checks
API_BASE_URL, NEXT_PUBLIC_API_BASE_URL, NEXT_PUBLIC_API_URL, and
NEXT_PUBLIC_BASE_URL; update the Error message in resolveApiOrigin (which calls
stripApiSuffix) to list all four accepted environment variables so users see
every valid option when the normalized origin is missing.
- Around line 11-27: The stripApiSuffix function currently only strips /api,
/api/vN, or /api/vN/{segment} but misses deeper paths; update stripApiSuffix to
remove any trailing "/api" optionally followed by "/v<digits>" and any further
path segments in one operation (i.e., use trimmedBaseUrl.replace(...) with a
regex that matches "/api" optionally followed by "/v" + digits and any following
path components) so it returns the base host for inputs like
"/api/v2/nested/path"; keep the initial trimming of whitespace and trailing
slashes and ensure the replacement is case-insensitive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5f55598-d85d-4e5f-a792-c250b4268964
📒 Files selected for processing (10)
src/vertex/app/changelog.mdsrc/vertex/components/features/auth/social-auth-section.tsxsrc/vertex/core/auth/authProvider.tsxsrc/vertex/core/auth/oauth-session.tssrc/vertex/core/hooks/useLogout.tssrc/vertex/core/hooks/useRefreshMetadataWithBanner.tssrc/vertex/core/hooks/useSites.tssrc/vertex/lib/api-routing.tssrc/vertex/lib/envConstants.tssrc/vertex/vertex.config.ts
✅ Files skipped from review due to trivial changes (1)
- src/vertex/app/changelog.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/vertex/core/hooks/useRefreshMetadataWithBanner.ts
- src/vertex/core/hooks/useSites.ts
BwanikaRobert
force-pushed
the
Migration-Update-Site-Management-Module-to-use-InfoBanners
branch
from
June 10, 2026 06:34
c072252 to
5443e3d
Compare
Contributor
|
New azure docs changes available for preview here |
Contributor
|
New azure analytics_platform changes available for preview here |
Contributor
|
New azure vertex 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
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?)
Migrates all user-facing notifications in the Site Management module from
ReusableToastand Sonnertoastto the centralizeduseBanner/useBannerWithDelay/useClipboardsystem.Hooks decoupled (
useSites.ts):Removed
ReusableToastfrom 4 mutation hooks —useApproximateCoordinates,useUpdateSiteDetails,useCreateSite,useRefreshSiteMetadata— and replaced with optionalonSuccess/onErrorcallback interfaces. Cache invalidation logic remains in the hooks; notification responsibility is delegated to the UI layer.Components updated:
edit-site-details-dialog.tsx— replaced 2 Sonnertoast.errorcalls withshowBanner(scoped: true); wired hook-level callbacks: error usesscoped: true(inline in dialog), success usesshowBannerWithDelay(scoped: false) after dialog closes.create-site-form.tsx— addeduseBanner/useBannerWithDelay; wireduseCreateSiteanduseApproximateCoordinateshook-level callbacks. Coordinate optimisation errors showscoped: true; site creation errors showscoped: true; success showsshowBannerWithDelay(scoped: false) after navigation fires.site-information-card.tsx— replaced clipboardReusableToastcalls with the shareduseClipboardhook (scoped: false).site-measurements-api-card.tsx— replaced 2 clipboardReusableToastcalls withuseClipboard; async error handling added.admin/sites/[id]/page.tsxandsites/[id]/page.tsx— wireduseRefreshSiteMetadatawith severity-aware banners (scoped: false): warning for partial refresh, info for already-complete, success for full refresh, error for failures.Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Summary by CodeRabbit
Improvements
New Features
Documentation