Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAlerts were removed from the main project tabs and added as a Project Settings → Alerts tab. Documentation and i18n were updated. Alerts UI was refactored from loader/deferred/Suspense to a fetcher-driven, prop-based component; route loader fetching, API exports for alerts, and keyboard shortcut handling for alerts were removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant SettingsUI as Project Settings UI
participant AlertsComp as ProjectAlerts Component
participant API as Server API
Note over SettingsUI,AlertsComp: Flow: Settings → Alerts tab (prop-driven, fetcher)
User->>SettingsUI: Click "Settings" → select "Alerts"
SettingsUI->>AlertsComp: Mount with props (projectId, projectRole)
AlertsComp->>API: GET /projects/:id/alerts (fetcher)
API-->>AlertsComp: 200 OK + alerts list
AlertsComp-->>SettingsUI: Render alerts list, pagination controls
User->>AlertsComp: Delete or Save alert
AlertsComp->>API: DELETE/POST /projects/:id/alerts
API-->>AlertsComp: 200 OK
AlertsComp->>API: Re-fetch alerts (fetcher) to refresh list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx (1)
289-297:⚠️ Potential issue | 🟠 MajorClear stale fetch errors on retry or success.
After
setError(fetcherError)runs once, nothing resets it. A later successful reload still leaveserrortruthy, so the component keeps renderingStatusPageuntil a full page refresh.Suggested fix
const loadAlerts = (take: number, skip: number) => { if (fetcher.state !== 'idle') return + setError(null) setIsSearchMode(true) fetcher.submit( { intent: 'get-project-alerts', take: String(take), skip: String(skip) }, { method: 'POST', action: `/projects/${id}` }, @@ const { intent, success, data, error: fetcherError } = fetcher.data if (success) { + setError(null) if (intent === 'get-project-alerts' && data) { const result = data as { results: Alerts[]; total: number } setAlerts(result.results) setTotal(result.total)Also applies to: 300-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx` around lines 289 - 297, When retrying or after a successful fetch you must clear the stale fetch error so the component can leave the StatusPage state; update the fetch flows (e.g., loadAlerts and the other fetcher handlers around lines 300-320) to call setError(undefined) before submitting a new request and call setError(undefined) when fetcher.state becomes 'idle' with a successful result. In practice, clear the error by invoking setError(undefined) immediately before fetcher.submit(...) in loadAlerts and in the other submit/reload handlers, and also reset error on successful response handling so previous fetcherError doesn't persist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx`:
- Around line 261-268: alerts is initialized to [] which conflates "not loaded
yet" with "loaded empty"; change the state to allow a null value (e.g.,
useState<Alerts[] | null>(null)) or add an explicit loaded flag, update all
setters (setAlerts, setTotal, setError) in the fetcher success/error paths to
set alerts to the fetched array (or [] on success) and to null/appropriate value
on start/error, and change render conditions to treat alerts === null (or loaded
flag false) as the loading state (use isLoading OR alerts === null) and only
show the empty-state hero when alerts !== null && alerts.length === 0; update
any other references to alerts (including spots currently rendering based on
alerts length) to follow the same check.
- Around line 312-315: When handling the 'delete-alert' intent in the
ProjectAlertsView, avoid reloading the same offset when the deleted alert was
the only item on the current page; instead calculate a new page: if page > 1 and
the current page will be empty after deletion (e.g., alertsOnPage === 1 or the
loadAlerts result is empty), decrement page by 1, update the page state, and
call loadAlerts with DEFAULT_ALERTS_TAKE and (newPage - 1) *
DEFAULT_ALERTS_TAKE; update references to page and loadAlerts so the UI shows
the previous page instead of an empty onboarding state.
In `@web/app/pages/Project/Settings/ProjectSettings.tsx`:
- Around line 409-416: The Alerts tab is visible to all cloud project members
but the page (ProjectAlertsView) currently only renders for projectRole ===
'owner', causing non-owners to see a dead upsell; either tighten the tab's
visible prop to only show for owners (update the tab entry with id: 'alerts' to
set visible: projectRole === 'owner') or relax the guard inside
ProjectAlertsView (replace the strict projectRole === 'owner' check with a
capability check or include admins/members) so the tab visibility and page
gating are consistent; locate the tab object with id 'alerts' in
ProjectSettings.tsx and the role check inside ProjectAlertsView.tsx to apply the
chosen fix.
---
Outside diff comments:
In `@web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx`:
- Around line 289-297: When retrying or after a successful fetch you must clear
the stale fetch error so the component can leave the StatusPage state; update
the fetch flows (e.g., loadAlerts and the other fetcher handlers around lines
300-320) to call setError(undefined) before submitting a new request and call
setError(undefined) when fetcher.state becomes 'idle' with a successful result.
In practice, clear the error by invoking setError(undefined) immediately before
fetcher.submit(...) in loadAlerts and in the other submit/reload handlers, and
also reset error on successful response handling so previous fetcherError
doesn't persist.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e6d1566-2cb2-4607-9f5d-1e6e1d00f04f
📒 Files selected for processing (12)
docs/content/docs/analytics-dashboard/alerts.mdxdocs/content/docs/sitesettings/embed-your-analytics-dashboard-into-your-website.mdxdocs/content/docs/sitesettings/get-traffic-alerts.mdxweb/app/lib/constants/index.tsweb/app/pages/Project/Settings/Alerts/ProjectAlertsSettings.tsxweb/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsxweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/View/ViewProject.helpers.tsxweb/app/pages/Project/View/ViewProject.tsxweb/app/pages/Project/View/components/ProjectSidebar.tsxweb/app/routes/projects.$id.tsxweb/public/locales/en.json
💤 Files with no reviewable changes (3)
- web/app/pages/Project/View/ViewProject.tsx
- web/app/lib/constants/index.ts
- web/app/routes/projects.$id.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx (2)
469-469: Consider showing a loading indicator whenalerts === null.When alerts haven't loaded yet, the component renders nothing (
null). Users have no visual feedback that data is being fetched. A simple loading spinner or skeleton would improve the experience, especially on slower connections.♻️ Example with loading state
<div className='mt-4'> - {alerts === null ? null : alerts.length === 0 ? ( + {alerts === null ? ( + <div className='flex justify-center py-8'> + {/* Add your preferred loading component here */} + <span className='text-gray-500'>{t('common.loading')}</span> + </div> + ) : alerts.length === 0 ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx` at line 469, The component currently returns null when alerts === null, leaving no feedback while data is loading; modify the render logic in ProjectAlertsView (the component handling the alerts variable) to show a loading indicator (e.g., a LoadingSpinner, Skeleton, or similar UI component) when alerts === null instead of null, falling back to the existing empty-state UI when alerts.length === 0 and the normal list when alerts is non-empty; locate the conditional around alerts (the expression starting with "{alerts === null ? null : alerts.length === 0 ? (") and replace the null branch with your chosen loading component so users see visual feedback during fetches.
314-321: Delete pagination fix is correct, but the explicitloadAlertscall is redundant.The page decrement logic correctly addresses the past review comment. However, since the
useEffectat lines 329-336 already triggersloadAlertswhenpagechanges, the explicit call on line 318 creates a redundant invocation pattern—the guard at line 290 prevents actual double fetches, but the intent would be clearer if you relied solely on the effect.♻️ Suggested simplification
} else if (intent === 'delete-alert') { toast.success(t('alertsSettings.alertDeleted')) if (page > 1 && alerts?.length === 1) { const newPage = page - 1 setPage(newPage) - loadAlerts(DEFAULT_ALERTS_TAKE, (newPage - 1) * DEFAULT_ALERTS_TAKE) } else { loadAlerts(DEFAULT_ALERTS_TAKE, (page - 1) * DEFAULT_ALERTS_TAKE) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx` around lines 314 - 321, Remove the redundant explicit loadAlerts call inside the branch that decrements the page: when you call setPage(newPage) let the existing useEffect that depends on page invoke loadAlerts; so in the if (page > 1 && alerts?.length === 1) branch only call setPage(newPage) and omit loadAlerts(DEFAULT_ALERTS_TAKE, (newPage - 1) * DEFAULT_ALERTS_TAKE), leaving the else branch’s explicit loadAlerts as-is for the case where page doesn’t change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx`:
- Line 469: The component currently returns null when alerts === null, leaving
no feedback while data is loading; modify the render logic in ProjectAlertsView
(the component handling the alerts variable) to show a loading indicator (e.g.,
a LoadingSpinner, Skeleton, or similar UI component) when alerts === null
instead of null, falling back to the existing empty-state UI when alerts.length
=== 0 and the normal list when alerts is non-empty; locate the conditional
around alerts (the expression starting with "{alerts === null ? null :
alerts.length === 0 ? (") and replace the null branch with your chosen loading
component so users see visual feedback during fetches.
- Around line 314-321: Remove the redundant explicit loadAlerts call inside the
branch that decrements the page: when you call setPage(newPage) let the existing
useEffect that depends on page invoke loadAlerts; so in the if (page > 1 &&
alerts?.length === 1) branch only call setPage(newPage) and omit
loadAlerts(DEFAULT_ALERTS_TAKE, (newPage - 1) * DEFAULT_ALERTS_TAKE), leaving
the else branch’s explicit loadAlerts as-is for the case where page doesn’t
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a8dd518-d693-4d7e-807f-dc6536d53c0b
📒 Files selected for processing (1)
web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx
6 tasks
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
1 participant
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.
Changes
#505
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
Documentation
New Features / UI
Bug Fixes / Behavior
Localization