perf: qos changes #477
perf: qos changes #477
Conversation
HarshPatel5940
added
documentation
|
Also, any UI changes should include an image screenshot in PR. |
|
|
UI has no change and notif env's being option in non-prod environments and handled gracefully |
JustinBenito
left a comment
JustinBenito
left a comment
There was a problem hiding this comment.
LGTM @HarshPatel5940 bro
Let me run this locally and merge this asap.
Cheers :D
|
This weekend is for you man , will check and merge ! |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughPR introduces production-only push notification functionality with environment guards across API routes and components, updates tooling from npm/yarn to pnpm, bumps version to 1.0.0, adds middleware for route normalization, extends environment variable declarations for VAPID keys and GitHub integration, and updates documentation with setup instructions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant PSub as PushSubscribe<br/>Component
participant SW as Service Worker
participant API as API Routes<br/>(save/remove)
participant WebPush as web-push<br/>Library
participant VAPID as Push Service
rect rgba(100, 150, 200, 0.5)
Note over Client,VAPID: Push Subscription Flow (Production Only)
Client->>PSub: User enables notifications
PSub->>SW: navigator.serviceWorker.ready
SW-->>PSub: ServiceWorkerRegistration
PSub->>API: POST /save-subscription<br/>(subscription object)
rect rgba(220, 100, 100, 0.5)
Note over API: Environment Guard
alt NODE_ENV !== 'production'
API-->>PSub: 403 Forbidden<br/>(dev warning logged)
PSub->>Client: Log warning, show user message
else NODE_ENV === 'production'
API->>WebPush: setVapidDetails()
API->>WebPush: saveSubscription(db)
WebPush-->>API: ✓ Saved
API-->>PSub: 200 OK
PSub->>Client: Notify success
end
end
end
rect rgba(100, 200, 150, 0.5)
Note over Client,VAPID: Notification Send Flow (Production Only)
Client->>WebPush: sendNotification()<br/>(subscription, payload)
alt NODE_ENV !== 'production'
WebPush-->>Client: false (dev message)
else NODE_ENV === 'production'
WebPush->>VAPID: Web Push Protocol
VAPID-->>SW: Push Event
SW->>Client: Display Notification
VAPID-->>WebPush: ✓ Delivered
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/pages/Communities/Community.tsx (1)
145-178: IncludeLine 145–153: Since
socialLinks, the divider should also consider it; otherwise communities with only a Reddit link won’t show the separator.💡 Proposed fix
- {(twitter || - linkedin || - github || - discord || - instagram || - bluesky || - mastodon || - telegram || - youtube) && <div className='mt-4 flex-shrink-0 border-t border-gray-100' />} + {(twitter || + linkedin || + github || + discord || + instagram || + bluesky || + mastodon || + telegram || + youtube || + reddit) && <div className='mt-4 flex-shrink-0 border-t border-gray-100' />}
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 16-17: The npm scripts "validate:events" and
"validate:communities" use require(), which fails under ESM; update both scripts
to read and parse the JSON using an ESM-compatible approach (e.g., use
fs.readFileSync + JSON.parse in the node -e string or use dynamic import()
inside an async script) so they no longer call require(), and keep the same
success log that prints the parsed array length for events and communities.
In `@pnpm-workspace.yaml`:
- Around line 1-3: pnpm-workspace.yaml is missing the required packages field
causing pnpm to fail; add a top-level packages entry (for a single-package repo
use packages: ['.']) to the file alongside the existing onlyBuiltDependencies
list so pnpm recognizes the workspace—update the file that currently contains
onlyBuiltDependencies, sharp, and unrs-resolver to include the new packages
field.
In `@src/app/Communities/page.tsx`:
- Around line 1-7: The import for Community in the Communities component uses
the wrong casing; update the import string in the Communities function from
'@/components/pages/communities/Community' to match the actual directory name
(e.g. '@/components/pages/Communities/Community') or rename the directory to
lowercase so the import and filesystem casing match; ensure the import of
Community and the exported Communities component remain unchanged.
In `@src/middleware.ts`:
- Around line 7-19: The current redirects map only handles two hardcoded
casings; update the logic in the middleware to normalize mixed-case variants by
comparing pathname.toLowerCase() against the set of allowed lowercased routes
(e.g., '/communities', '/archive') and only redirect when pathname !==
lowercased value. Replace the hardcoded redirects usage with a check like: const
lower = pathname.toLowerCase(); if (allowedRoutes.has(lower) && pathname !==
lower) { const url = request.nextUrl.clone(); url.pathname = lower; return
NextResponse.redirect(url, 301); } — reference symbols: pathname,
request.nextUrl.clone(), NextResponse.redirect, and the current redirects map
(remove or repurpose it).
🧹 Nitpick comments (4)
src/app/api/remove-subscription/route.ts (1)
17-23: Guard may still allow preview/staging whenNODE_ENV=production.On many platforms, preview/staging deployments set
NODE_ENVto"production", so this won’t actually disable push outside real prod. Consider gating on an explicit env flag (e.g.,PUSH_NOTIFICATIONS_ENABLED) or another environment selector you control.💡 Example adjustment
- if (process.env.NODE_ENV !== 'production') { + const pushEnabled = + process.env.NODE_ENV === 'production' && + process.env.PUSH_NOTIFICATIONS_ENABLED === 'true'; + if (!pushEnabled) { return NextResponse.json( { error: 'Push notifications are disabled in non-production environments' }, { status: 403 } ); }src/app/api/save-subscription/route.ts (1)
31-37:NODE_ENValone may not enforce “non-production” blocking.If preview/staging also use
NODE_ENV="production", this check won’t prevent pushes there. Consider an explicit flag (e.g.,PUSH_NOTIFICATIONS_ENABLED) or another environment selector to make the intent reliable.💡 Example adjustment
- if (process.env.NODE_ENV !== 'production') { + const pushEnabled = + process.env.NODE_ENV === 'production' && + process.env.PUSH_NOTIFICATIONS_ENABLED === 'true'; + if (!pushEnabled) { return NextResponse.json( { error: 'Push notifications are disabled in non-production environments' }, { status: 403 } ); }src/lib/webpush.ts (1)
45-49: Consider returningtrueinstead offalsein dev mode.Returning
falseindicates failure, but in dev mode the notification wasn't actually "failed" — it was intentionally skipped. This could confuse callers or affect metrics ifsendBulkNotificationscounts this as a failure.Alternative: return true to indicate "handled successfully (skipped)"
if (!isProduction) { console.log('[DEV] Notification disabled in non-production environment:', payload); - return false; + return true; // Treated as success (intentionally skipped) }Alternatively, document that
falseis expected in non-production environments.src/components/PushSubscribe.tsx (1)
163-166: Non-production 403 handling doesn't prevent error state on other failures.The
unsubscribeFromNotificationsfunction handles 403 gracefully in dev, but other non-OK responses are silently ignored. Consider logging or handling other error statuses.Optional: log other error responses
// Handle non-production environment gracefully if (!response.ok && response.status === 403 && !isProduction) { console.warn('[DEV] Push notifications disabled in non-production environment'); + } else if (!response.ok) { + console.warn('Failed to remove subscription from server:', response.status); }
JustinBenito
left a comment
JustinBenito
left a comment
There was a problem hiding this comment.
@HarshPatel5940 finally reviewed it XD
Please make these changes and lets merge it asap!
| npm install | ||
| # or | ||
| yarn install | ||
| pnpm install |
2 weekends are gone man XD |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@JustinBenito gm bro |
4 participants
What was addressed?
eval()security riskSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.