Mobile app prerequisites#189
Merged
Merged
Conversation
- Updated tests for GET /api/servers/public to use NextRequest for request creation. - Simplified mock implementations in chat hooks tests for better readability. - Enhanced error handling in notifications and chat hooks to log errors more effectively. - Improved API response structure by changing 'total' to 'count' in public servers response. - Added CORS and rate limiting logic to proxy handling for API routes. - Removed middleware file and integrated its functionality into the proxy. - Updated various components to improve accessibility and user experience. - Refactored rate limit configuration to use parsed environment variables. - Cleaned up unused variables and improved code readability across multiple files.
FirepitProject ID: Tip Preview deployments create instant URLs for every branch and commit |
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds header-first session auth and a SessionUser type, centralizes typed API error responses, implements in-memory rate limiting and CORS-aware API proxying, hardens realtime subscription lifecycle and pool teardown, converts public servers API to cursor pagination with search, extends instance/version endpoints, and updates many hooks, UI bits, tests, and deps (v1.8.0 → v1.9.0). ChangesAuth & API error contract
Rate limiting & proxy CORS
Realtime subscription lifecycle & pool
Realtime hook wiring & guarded cleanup
API routes, pagination & metadata
UI, components & misc
Package, docs & tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
|
Merged
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/realtime-pool.ts (1)
48-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
toUnsubscribeFndetaches async teardown and breaks ordering guarantees.At Line 69/Line 70, the wrapper returns immediately and does not await the underlying
unsubscribe/closePromise. That makesawaitcalls (e.g., Line 235) complete before teardown actually finishes, so subscribe/teardown operations can interleave.Proposed fix
-function toUnsubscribeFn(subscription: unknown): () => void { +function toUnsubscribeFn(subscription: unknown): () => Promise<void> { @@ - return () => { - void Promise.resolve(unsubscribe()).catch((error) => { - logger.warn( - "Realtime subscription unsubscribe failed in wrapper", - { - error: - error instanceof Error - ? error.message - : String(error), - }, - ); - }); - }; + return async () => { + try { + await Promise.resolve(unsubscribe()); + } catch (error) { + logger.warn( + "Realtime subscription unsubscribe failed in wrapper", + { + error: + error instanceof Error + ? error.message + : String(error), + }, + ); + } + }; }🤖 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/lib/realtime-pool.ts` around lines 48 - 81, toUnsubscribeFn currently wraps object unsubscribe/close calls but detaches the async teardown by resolving the promise and swallowing completion, which breaks ordering guarantees; change the returned wrapper to be an async function that awaits the underlying unsubscribe/close call (i.e., await unsubscribe()) and let it propagate or catch+rethrow as appropriate so callers can await teardown completion; keep the existing logger.warn but only log inside a catch after awaiting, and ensure the wrapped value (unsubscribe variable) remains the bound function used by the async wrapper in toUnsubscribeFn.src/lib/auth-server.ts (1)
28-131: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExtract duplicate user validation and transformation logic.
Both
getSessionFromHeader(lines 54-77) andgetSessionFromCookie(lines 104-127) contain identical logic for:
- Validating the user object structure (checking
$id,name,- Rejecting the system sender user
- Constructing the
SessionUserobjectThis duplication increases maintenance burden and the risk that future changes won't be applied consistently.
♻️ Refactor to eliminate duplication
Add a helper function to handle common validation and transformation:
+function validateAndTransformUser( + user: unknown, + systemSenderUserId: string | null, +): SessionUser | null { + if ( + !user || + typeof user !== "object" || + !("$id" in user) || + typeof user.$id !== "string" || + typeof user.name !== "string" || + typeof user.email !== "string" + ) { + return null; + } + + if (systemSenderUserId && user.$id === systemSenderUserId) { + return null; + } + + return { + $id: user.$id, + name: user.name, + email: user.email, + $createdAt: + "$createdAt" in user && typeof user.$createdAt === "string" + ? user.$createdAt + : undefined, + }; +} + async function getSessionFromHeader( endpoint: string, project: string, systemSenderUserId: string | null, ): Promise<SessionUser | null> { try { const headerStore = await headers(); const authHeader = headerStore.get("Authorization"); if (!authHeader?.startsWith("Bearer ")) { return null; } const token = authHeader.slice(7); if (!token) { return null; } const client = new Client() .setEndpoint(endpoint) .setProject(project) .setSession(token); const account = new Account(client); const user = await account.get().catch(() => null); - if ( - !user || - typeof user !== "object" || - !("$id" in user) || - typeof user.$id !== "string" || - typeof user.name !== "string" || - typeof user.email !== "string" - ) { - return null; - } - - if (systemSenderUserId && user.$id === systemSenderUserId) { - return null; - } - - return { - $id: user.$id, - name: user.name, - email: user.email, - $createdAt: - "$createdAt" in user && typeof user.$createdAt === "string" - ? user.$createdAt - : undefined, - }; + return validateAndTransformUser(user, systemSenderUserId); } catch { return null; } } async function getSessionFromCookie( endpoint: string, project: string, systemSenderUserId: string | null, ): Promise<SessionUser | null> { try { const cookieStore = await cookies(); const sessionCookie = cookieStore.get(`a_session_${project}`); if (!sessionCookie?.value) { return null; } const client = new Client() .setEndpoint(endpoint) .setProject(project) .setSession(sessionCookie.value); const account = new Account(client); const user = await account.get().catch(() => null); - if ( - !user || - typeof user !== "object" || - !("$id" in user) || - typeof user.$id !== "string" || - typeof user.name !== "string" || - typeof user.email !== "string" - ) { - return null; - } - - if (systemSenderUserId && user.$id === systemSenderUserId) { - return null; - } - - return { - $id: user.$id, - name: user.name, - email: user.email, - $createdAt: - "$createdAt" in user && typeof user.$createdAt === "string" - ? user.$createdAt - : undefined, - }; + return validateAndTransformUser(user, systemSenderUserId); } catch { return 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/lib/auth-server.ts` around lines 28 - 131, Extract the repeated user validation/transformation into a helper like validateAndTransformUser(user: unknown, systemSenderUserId: string | null): SessionUser | null and call it from both getSessionFromHeader and getSessionFromCookie after you fetch the user via new Account(client).get(); the helper should check that user is an object with string $id, name, email, reject if user.$id === systemSenderUserId, and return the SessionUser shape including optional $createdAt when it's a string; replace the identical inline checks/returns in getSessionFromHeader and getSessionFromCookie with a single call to this helper and return its result.
🤖 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 `@package.json`:
- Around line 24-26: The package.json scripts "update:force", "update:exact",
and "update:check" are inconsistent with the project's current caret (^)
dependency ranges; decide and enforce a single versioning strategy: either (A)
keep caret ranges and remove --save-exact from "update:exact" or rename it
(e.g., to "update:pin") to avoid confusion, (B) convert existing dependencies to
exact versions and keep --save-exact, or (C) keep both scripts but add a short
README note explaining when to use "update:force" (latest caret/semver updates)
vs "update:exact" (strict exact pinning). Update the package.json scripts and
the repository docs accordingly and reference the script names ("update:force",
"update:exact", "update:check") when making changes so contributors understand
the chosen policy.
In `@src/app/api/servers/public/route.ts`:
- Around line 83-93: The pagination metadata is computed after applying a
client-side search filter, which can make hasMore and nextCursor incorrect; move
the search filtering step so that (doc.name) filtering on serverDocuments
happens immediately after you map/collect the raw results and before you compute
hasMore and slice to limit, and then compute hasMore and nextCursor from the
filtered array (serverDocuments) rather than from the pre-filtered subset;
ensure the logic around limit, hasMore and nextCursor (the variables
serverDocuments, search, hasMore, limit, nextCursor) uses the filtered length so
cursors remain valid even when mapping or filtering reduces matches.
- Around line 131-140: In the catch block of the route handler (where
NextResponse.json is used) don't expose error.message to clients; instead log
the raw error server-side (e.g., console.error(error) or
processLogger.error(error)) and return a generic 500 response body such as {
error: "Internal server error" } or { error: "Failed to fetch servers" } while
keeping the status: 500; update the catch to remove the ternary that returns
error.message and ensure the real error is only written to server logs.
In `@src/app/page.tsx`:
- Line 81: The element building its className uses Tailwind v3 syntax
`bg-gradient-to-r`; update it to v4 syntax by replacing `bg-gradient-to-r` with
`bg-linear-to-r` and ensure explicit color stop classes are present (e.g.
`from-...` and `to-...`); if your gradient colors come from feature.accentClass,
modify that value (or append the stops in the className where used) so it
provides the `from-<color>` and `to-<color>` Tailwind classes alongside
`bg-linear-to-r` instead of relying on `bg-gradient-to-r`.
In `@src/lib/rate-limit.ts`:
- Around line 23-34: The current parsing for PARSED_RATE_LIMIT_AUTH_WINDOW_MS,
PARSED_RATE_LIMIT_AUTH_MAX, PARSED_RATE_LIMIT_API_WINDOW_MS, and
PARSED_RATE_LIMIT_API_MAX accepts negative or invalid numbers because it relies
on `parseInt(...) || default`; change each to explicitly parse the env var
(e.g., Number.parseInt or Number(...)), then validate that the result is a
finite integer and > 0 (or >= 0 if zero is allowed for your semantics); if
validation fails, fall back to DEFAULT_AUTH_CONFIG.windowMs /
DEFAULT_AUTH_CONFIG.maxRequests or DEFAULT_API_CONFIG.windowMs /
DEFAULT_API_CONFIG.maxRequests respectively, and ensure you do not accept NaN,
Infinity, or negative values.
In `@src/proxy.ts`:
- Around line 127-135: The generated requestId (requestId) is only being added
to corsHeaders and not forwarded to downstream handlers; update the forwarded
request headers passed into NextResponse.next() so they include the generated
"X-Request-ID" — create a mutable Headers copy from request.headers,
set("X-Request-ID", requestId) on it, and pass that Headers instance into
NextResponse.next({ request: { headers: newHeaders } }) so route handlers using
the helpers in src/lib/api-response.ts can access the same ID.
---
Outside diff comments:
In `@src/lib/auth-server.ts`:
- Around line 28-131: Extract the repeated user validation/transformation into a
helper like validateAndTransformUser(user: unknown, systemSenderUserId: string |
null): SessionUser | null and call it from both getSessionFromHeader and
getSessionFromCookie after you fetch the user via new Account(client).get(); the
helper should check that user is an object with string $id, name, email, reject
if user.$id === systemSenderUserId, and return the SessionUser shape including
optional $createdAt when it's a string; replace the identical inline
checks/returns in getSessionFromHeader and getSessionFromCookie with a single
call to this helper and return its result.
In `@src/lib/realtime-pool.ts`:
- Around line 48-81: toUnsubscribeFn currently wraps object unsubscribe/close
calls but detaches the async teardown by resolving the promise and swallowing
completion, which breaks ordering guarantees; change the returned wrapper to be
an async function that awaits the underlying unsubscribe/close call (i.e., await
unsubscribe()) and let it propagate or catch+rethrow as appropriate so callers
can await teardown completion; keep the existing logger.warn but only log inside
a catch after awaiting, and ensure the wrapped value (unsubscribe variable)
remains the bound function used by the async wrapper in toUnsubscribeFn.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 492bfb63-db2a-42f1-be3f-d701f41a477b
⛔ Files ignored due to path filters (2)
.env.local.exampleis excluded by none and included by nonebun.lockis excluded by!**/*.lockand included bybun.lock
📒 Files selected for processing (33)
package.jsonsrc/__tests__/api-routes/servers-public.test.tssrc/__tests__/chat-hooks-thread-read-wiring.test.tsxsrc/__tests__/chat-hooks-useConversations.test.tssrc/__tests__/chat-hooks-useDirectMessages.test.tssrc/__tests__/hooks/useNotifications.test.tssrc/__tests__/setup.tssrc/app/(auth)/login/login-form.tsxsrc/app/(auth)/login/page.tsxsrc/app/api/instance/route.tssrc/app/api/messages/[messageId]/thread/route.tssrc/app/api/servers/[serverId]/mentionable-roles/route.tssrc/app/api/servers/public/route.tssrc/app/api/version/route.tssrc/app/chat/hooks/useConversations.tssrc/app/chat/hooks/useInbox.tssrc/app/chat/hooks/useStatusSubscription.tssrc/app/chat/page.tsxsrc/app/manifest.tssrc/app/notifications/page.tsxsrc/app/page.tsxsrc/components/header.tsxsrc/components/mention-autocomplete.tsxsrc/components/notifications-menu.tsxsrc/components/settings-section-nav.tsxsrc/hooks/useNotifications.tssrc/index.csssrc/lib/api-response.tssrc/lib/auth-server.tssrc/lib/rate-limit.tssrc/lib/realtime-error-suppression.tssrc/lib/realtime-pool.tssrc/proxy.ts
💤 Files with no reviewable changes (1)
- src/hooks/useNotifications.ts
…onents - Refactored NotificationsMenu component for better readability and consistency in formatting. - Updated Providers component to enhance clarity in QueryClient instantiation. - Modified SettingsSectionNav to improve transition effects on active states. - Cleaned up AuthContext by organizing imports and enhancing subscription management logic. - Introduced validateAndTransformUser function in auth-server for user validation. - Enhanced rate-limit logic with a new function to parse environment variables. - Added idle teardown listeners in realtime-pool for better resource management. - Improved request handling in proxy to ensure consistent request IDs. - Created tests for rate-limit environment variable parsing to ensure defaults are applied correctly. - Added ResourceHints component for preconnecting to external services.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (1)
src/proxy.ts (1)
49-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow API-route detection to
/apiand/api/*only.Line 49 uses
startsWith("/api"), which also matches non-API paths like/apiary. Those requests would incorrectly bypass the non-API auth branch.Suggested fix
- if (pathname.startsWith("/api")) { + if (pathname === "/api" || pathname.startsWith("/api/")) {🤖 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/proxy.ts` around lines 49 - 51, The API-route check using pathname.startsWith("/api") is too broad and matches paths like "/apiary"; update the conditional in src/proxy.ts to only treat requests as API routes when the pathname is exactly "/api" or begins with "/api/". Replace the current check around the variables rateLimitEnabled/rateLimitHeaders (the block using pathname.startsWith("/api")) with a tightened predicate (e.g., pathname === "/api" || pathname.startsWith("/api/")) so non-API routes do not fall into the API auth/rate-limit branch.
🤖 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/__tests__/api-routes/servers-public.test.ts`:
- Around line 48-50: Add tests exercising the new cursor-pagination path by
updating the test helper createRequest and the test cases in
servers-public.test.ts to send requests with a query string containing ?cursor=
(use createRequest("http://localhost/api/servers/public?cursor=<value>")) and
assert that the JSON response includes nextCursor and count fields and that
items correspond to the expected second-page slice; additionally add a test that
performs a search query plus pagination (e.g.,
createRequest("http://localhost/api/servers/public?search=<term>&cursor=<boundary>"))
to assert that the search results respect the pagination boundary and that
nextCursor/count behave correctly. Use the existing request helper createRequest
and the same handler invocation used elsewhere in this test file so the new
cases run with the current fixtures and assert both presence and correctness of
nextCursor and count.
In `@src/__tests__/middleware.test.ts`:
- Around line 220-234: The test "should forward the request ID to downstream
handlers" restores the NextResponse.next spy only at the end, so if an assertion
throws the spy isn't restored; wrap the test logic that calls middleware() and
asserts forwarded headers in a try/finally and move nextSpy.mockRestore() into
the finally block so NextResponse.next is always restored; reference the test
case, the nextSpy created with vi.spyOn(NextResponse, "next"), and the
middleware() invocation to locate where to add the try/finally.
In `@src/__tests__/rate-limit-env.test.ts`:
- Around line 9-37: The tests mutate process.env (e.g., setting
RATE_LIMIT_AUTH_WINDOW_MS/ MAX and RATE_LIMIT_API_WINDOW_MS/ MAX) and do not
restore it; capture a snapshot of process.env before each test (or in a
top-level beforeEach) and restore the original env in an afterEach so tests
don't leak state, or explicitly revert the specific keys after each it; update
the test file around the uses of importFreshRateLimitModule and rateLimitRequest
to ensure process.env is reset after each test run.
In `@src/__tests__/realtime-pool.test.ts`:
- Around line 191-211: The test must prove the wrapper awaits the unsubscribe
promise: change unsubscribeSpy to return a deferred promise (create a deferred
with its resolve function), have mockRealtimeSubscribe resolve to that
unsubscribeSpy, call wrappedSubscribe and capture the returned unsubscribe
promise but do not await it immediately—assert the spy has not been called yet
and the wrapper's unsubscribe is still pending, then resolve the deferred and
await the unsubscribe promise, finally assert unsubscribeSpy was called once;
reference getSharedRealtime, mockRealtimeSubscribe, wrappedSubscribe and
unsubscribe in your changes.
In `@src/app/api/servers/`[serverId]/mentionable-roles/route.ts:
- Around line 169-175: The payload currently forwards the optional field
doc.mentionable directly in the mentionableRoles mapping which can be undefined;
change the mapping in the mentionableRoles creation (the map that builds
id/name/color/mentionable/memberCount) to coerce mentionable to a strict boolean
(e.g., use Boolean(doc.mentionable) or !!doc.mentionable) so the API always
returns true/false for the mentionable property.
In `@src/app/api/servers/public/route.ts`:
- Around line 102-121: The current pagination uses servers.at(-1) which can be
earlier than the final fetched document if mapServerDocument() threw; instead
derive nextCursor from the last raw document in serverDocuments so failures at
the tail don't reappear. Replace the logic that computes last/nextCursor to use
serverDocuments.at(-1) (e.g., const lastDoc = serverDocuments.at(-1)) and set
nextCursor = hasMore && lastDoc ? String(lastDoc.$id) : null; keep failedIds
logging and mapping logic unchanged.
In `@src/app/invite/`[code]/InvitePreviewClient.tsx:
- Around line 35-40: The effect that auto-joins is missing dependencies and can
close over stale values: include both joining and handleJoin in the dependency
array and prevent repeat runs by tracking if we've already auto-joined. Wrap the
existing handleJoin function in useCallback (with its current dependencies) so
it can be safely referenced from the effect, add a hasAutoJoinedRef
(useRef<boolean>(false)) to short-circuit subsequent effects, and update the
useEffect dependencies to [searchParams, isAuthenticated, joining, handleJoin];
inside the effect check auto param, isAuthenticated, !joining and
!hasAutoJoinedRef.current before calling handleJoin and set
hasAutoJoinedRef.current = true after invoking it.
In `@src/app/page.tsx`:
- Around line 230-241: Replace the three paragraphs that render bullet
characters with a semantic unordered list: convert the three <p> elements into a
single <ul> containing three <li> items (remove the leading "•" characters),
ensure the list is placed in the same JSX location where the current paragraph
block is rendered, and remove any role attributes used to emulate list behavior
so the native <ul>/<li> semantics are relied upon instead.
In `@src/components/chat-input.tsx`:
- Around line 112-131: The code currently refetches
`/api/servers/${serverId}/mentionable-roles` inside the mentionQuery handler on
every debounced keystroke; instead, fetch mentionable roles once per server and
filter locally. Implement a per-server cache (e.g., a ref or state object keyed
by serverId) and change the block around mentionQuery/serverId so that on
serverId change or cache miss you call fetch and populate the cache, then always
derive filtered results by applying the existing .filter(...) to the cached
roles and call setMentionableRoles; only call the network when the cache does
not contain roles for the current serverId.
- Around line 125-130: Role ID comparison is currently case-sensitive because
role.id is compared directly to the lowercased query; change the filter so both
sides are normalized by using role.id.toLowerCase().includes(queryLower)
(alongside the existing role.name.toLowerCase().includes(queryLower)), ensuring
you call toLowerCase() safely on role.id within the arrow filter (role:
MentionableRole) => ... so ID matching is case-insensitive.
- Around line 236-242: The warning currently logs the full selectable object
which may contain PII; update the handleMentionSelect logger.warn call to stop
including the raw selectable and instead emit only minimal diagnostics such as
typeof selectable and a small non-sensitive shape indicator (e.g., whether it is
an array/object and a list of keys or a boolean hasId flag) so no
identifiers/display names are logged; locate the logger.warn inside
handleMentionSelect and replace the selectable payload with these minimal
diagnostics.
In `@src/components/header.tsx`:
- Around line 278-288: handleCustomStatusSubmit currently assumes
updateUserStatus will throw on failure; instead await its result and handle
non-throwing failures explicitly by checking the returned value (e.g.,
result.success or result.error) from updateUserStatus, calling toast.error(...)
and not calling setAccountMenuOpen(false) when the update indicates failure, and
only closing the menu when the response denotes success; reference
handleCustomStatusSubmit, updateUserStatus, setAccountMenuOpen, and toast.error
when making this change.
In `@src/contexts/auth-context.tsx`:
- Around line 292-308: When existing.update(...) throws, the old live
subscription is left open; modify the catch block that follows the call to
existing.update on subscriptionRef.current so it explicitly closes/unsubscribes
the old subscription (call and await existing.close(...) or
existing.unsubscribe(...) if present, or fallback to any provided teardown
method), then clear subscriptionRef.current before proceeding to recreate a new
subscription; ensure you reference subscriptionRef.current and the existing
object around the update() call and handle both sync and async close methods
safely.
In `@src/lib/auth-server.ts`:
- Around line 67-75: The current check uses authHeader?.startsWith("Bearer ")
which rejects valid but differently-cased schemes; update the Authorization
parsing in the authHeader handling (where headerStore.get("Authorization") is
read, the authHeader check and token extraction) to compare the scheme
case-insensitively (e.g., split authHeader on whitespace and compare the scheme
lowercased to "bearer" or use a case-insensitive regex) and then extract the
token from the second segment (ensure you still return null when token is
missing). This fixes acceptance of "bearer", "BEARER", or mixed-case schemes
while keeping existing null returns when no token is present.
In `@src/lib/rate-limit.ts`:
- Around line 70-86: The getClientIp function currently trusts the first value
in x-forwarded-for which can be spoofed; update getClientIp to prefer
direct/proxy-supplied addresses (e.g. x-real-ip or cloud-provider headers like
cf-connecting-ip) and only fall back to x-forwarded-for when the request is
known to come through a trusted proxy: implement validation using a proper IP
check (net.isIP or an IP regex), ignore private/loopback ranges when determining
the client address, and allow configuration of trusted proxy IPs or a
TRUSTED_PROXIES env var so x-forwarded-for is only honored when the immediate
peer is in that trusted list; reference and modify getClientIp to perform these
checks and return "unknown" if no validated public IP is found.
- Around line 110-151: The rate-limit key currently only uses the client
identifier so auth and API requests share one counter; change the key to include
the request scope (auth vs api) so they have separate counters—either update
rateLimitRequest to prepend a scope prefix (e.g., "auth:" or "api:") to the
clientIp before calling checkRateLimit, or modify checkRateLimit to accept a
second scope parameter and include that scope in the generated key; adjust uses
of checkRateLimit and the key construction in checkRateLimit accordingly
(functions to locate: rateLimitRequest and checkRateLimit).
In `@src/lib/realtime-pool.ts`:
- Around line 19-37: The current installIdleTeardownListeners uses teardown that
always calls disposeSharedRealtime on blur/pagehide/visibilitychange which can
silently stop updates; change teardown to only call disposeSharedRealtime when
there are no active mounted consumers (e.g., check a module-level
activeSubscriptionCount or similar counter) and when the document is actually
hidden/page is unloading. Specifically, add or use an existing
activeSubscriptionCount (increment on subscriber mount, decrement on unmount),
and in installIdleTeardownListeners replace the unconditional teardown with a
guard: if (activeSubscriptionCount === 0 && document.visibilityState ===
"hidden") { disposeSharedRealtime(); } and avoid disposing on plain blur unless
activeSubscriptionCount is 0 and pagehide/visibility confirm unload. Ensure you
update subscriber mount/unmount code to maintain the counter so
disposeSharedRealtime is only called when truly idle.
In `@src/proxy.ts`:
- Around line 52-81: The rate-limiting block currently runs before CORS
preflight handling, so OPTIONS requests can be throttled; update the middleware
to detect preflight (request.method === "OPTIONS") and short-circuit before
calling rateLimitRequest so preflight responses are returned/forwarded without
decrementing or blocking via rateLimitRequest; locate the rateLimitEnabled check
and the rateLimitRequest call in src/proxy.ts and ensure OPTIONS handling occurs
above/before that block (or that rateLimitRequest is skipped for OPTIONS),
returning an appropriate CORS/NextResponse for preflight.
---
Duplicate comments:
In `@src/proxy.ts`:
- Around line 49-51: The API-route check using pathname.startsWith("/api") is
too broad and matches paths like "/apiary"; update the conditional in
src/proxy.ts to only treat requests as API routes when the pathname is exactly
"/api" or begins with "/api/". Replace the current check around the variables
rateLimitEnabled/rateLimitHeaders (the block using pathname.startsWith("/api"))
with a tightened predicate (e.g., pathname === "/api" ||
pathname.startsWith("/api/")) so non-API routes do not fall into the API
auth/rate-limit branch.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: afb91ebf-0f44-4cc9-a9d3-79d704d77477
📒 Files selected for processing (28)
README.mdpackage.jsonsrc/__tests__/api-routes/servers-public.test.tssrc/__tests__/components/conversation-list.test.tsxsrc/__tests__/middleware.test.tssrc/__tests__/rate-limit-env.test.tssrc/__tests__/realtime-pool.test.tssrc/app/api/servers/[serverId]/mentionable-roles/route.tssrc/app/api/servers/public/route.tssrc/app/chat/components/ConversationList.tsxsrc/app/chat/hooks/useStatusSubscription.tssrc/app/head.tsxsrc/app/invite/[code]/InvitePreviewClient.tsxsrc/app/layout.tsxsrc/app/manifest.tssrc/app/page.tsxsrc/components/chat-input.tsxsrc/components/header.tsxsrc/components/mention-autocomplete.tsxsrc/components/notifications-menu.tsxsrc/components/providers.tsxsrc/components/resource-hints.tsxsrc/components/settings-section-nav.tsxsrc/contexts/auth-context.tsxsrc/lib/auth-server.tssrc/lib/rate-limit.tssrc/lib/realtime-pool.tssrc/proxy.ts
💤 Files with no reviewable changes (1)
- src/app/head.tsx
…e middleware error handling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/chat-input.tsx (1)
101-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCancel in-flight mention lookups before writing state.
The current cleanup function only clears the debounce timer. When
mentionQuerychanges rapidly, an older fetch can still resolve after a newer one completes, callingsetAvailableUsers()orsetMentionableRoles()with stale results and overwriting the fresh data. UseAbortControllerto cancel the fetch request and add guards to prevent state updates after cancellation.Suggested fix
useEffect(() => { if (!showMentionAutocomplete) { setAvailableUsers([]); setMentionableRoles([]); setIsLoadingUsers(false); return; } + const controller = new AbortController(); + let cancelled = false; + // Show loading immediately when autocomplete is shown setIsLoadingUsers(true); // If query is empty, fetch all users const fetchUsersAndRoles = async () => { try { const query = mentionQuery || ""; const response = await fetch( `/api/users/search?q=${encodeURIComponent(query)}&limit=10`, + { signal: controller.signal }, ); + if (cancelled) { + return; + } if (response.ok) { const data = await response.json(); - setAvailableUsers(data.users || []); + if (!cancelled) { + setAvailableUsers(data.users || []); + } } else { - setAvailableUsers([]); + if (!cancelled) { + setAvailableUsers([]); + } } const queryLower = query.toLowerCase(); if (serverId) { try { let cachedRoles = mentionableRolesCacheRef.current[serverId]; if (!cachedRoles) { const rolesResponse = await fetch( `/api/servers/${serverId}/mentionable-roles`, + { signal: controller.signal }, ); + if (cancelled) { + return; + } if (rolesResponse.ok) { const rolesData = await rolesResponse.json(); cachedRoles = (rolesData.roles || []).map( (role: Omit<MentionableRole, "type">) => ({ ...role, type: "role" as const, }), ); mentionableRolesCacheRef.current[serverId] = cachedRoles; } else { cachedRoles = []; } } const typedRoles = cachedRoles.filter( (role: MentionableRole) => role.name.toLowerCase().includes(queryLower) || role.id.toLowerCase().includes(queryLower), ); - setMentionableRoles(typedRoles); - } catch { - setMentionableRoles([]); + if (!cancelled) { + setMentionableRoles(typedRoles); + } + } catch (error) { + if ( + !cancelled && + !( + error instanceof DOMException && + error.name === "AbortError" + ) + ) { + setMentionableRoles([]); + } } } else { - setMentionableRoles([]); + if (!cancelled) { + setMentionableRoles([]); + } } - } catch { - setAvailableUsers([]); + } catch (error) { + if ( + !cancelled && + !( + error instanceof DOMException && + error.name === "AbortError" + ) + ) { + setAvailableUsers([]); + setMentionableRoles([]); + } } finally { - setIsLoadingUsers(false); + if (!cancelled) { + setIsLoadingUsers(false); + } } }; const debounce = setTimeout(() => { void fetchUsersAndRoles(); }, 150); - return () => clearTimeout(debounce); + return () => { + cancelled = true; + controller.abort(); + clearTimeout(debounce); + }; }, [mentionQuery, showMentionAutocomplete, serverId]);🤖 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/components/chat-input.tsx` around lines 101 - 155, The fetchUsersAndRoles function can race when mentionQuery changes; create an AbortController for each call (or separate controllers for the user fetch and the roles fetch), pass controller.signal to both fetch calls, store the controller so the debounce cleanup can call controller.abort(), and in the fetch promise chains catch AbortError and skip calling setAvailableUsers/setMentionableRoles; additionally, before calling the setters verify the request wasn't aborted (e.g., check controller.signal.aborted or compare the current mentionQuery/serverId) so stale responses don't overwrite fresh state; ensure the finally block still sets setIsLoadingUsers(false) only for non-aborted flows or handles aborted flows appropriately.
🤖 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/app/api/servers/`[serverId]/mentionable-roles/route.ts:
- Around line 136-155: The Appwrite queries use unindexed or non-existent
fields: the roles query filters on "mentionable" and the role assignments query
filters on "roleId" while the collection stores "roleIds" (array) and neither
field has indexes; either add indexes for "mentionable" on env.collections.roles
and for "roleIds" on env.collections.roleAssignments in
scripts/setup-appwrite.ts, or change the code around getAllDocumentsPaginated
where mentionableRoleDocs and mentionableAssignments are built to fetch broader
results and perform the "mentionable" and roleId->roleIds membership filtering
in-memory (use roleIds array membership checks against mentionableRoleIds) to
avoid relying on unsupported Appwrite filters.
In `@src/app/invite/`[code]/InvitePreviewClient.tsx:
- Line 92: The decorative icon elements in InvitePreviewClient.tsx (the <Users
... /> and <Loader2 ... /> JSX components) should be hidden from assistive tech:
update each occurrence of Users and Loader2 (the instances near where the user
count and loading indicators are rendered) to include aria-hidden="true" so
screen readers skip them and do not duplicate the adjacent text labels.
In `@src/lib/auth-server.ts`:
- Around line 60-113: Extract the duplicated Appwrite client/account lookup into
a new helper getSessionForToken(endpoint, project, token, systemSenderUserId):
have it create the Client (new
Client().setEndpoint(endpoint).setProject(project).setSession(token)), construct
Account(client), call account.get() (catching errors to return null) and finally
return validateAndTransformUser(user, systemSenderUserId) or null; then simplify
getSessionFromHeader and getSessionFromCookie to only obtain the token (from
Authorization header or cookie) and call getSessionForToken with that token,
preserving the existing try/catch behavior and null returns used in
getSessionFromHeader/getSessionFromCookie.
In `@src/lib/rate-limit.ts`:
- Line 8: The current in-memory Map rateLimitStore (and its cleanup interval) is
per-process and won’t work correctly in multi-instance or serverless
deployments; update the code to (1) add a clear comment/docstring near
rateLimitStore and the cleanup logic explaining this limitation and that it’s
only suitable for single-process/local use, and (2) refactor the rate limiting
to use an interchangeable store interface (e.g., IRateLimitStore with methods
like get/set/delete/cleanup) and provide a Redis-backed implementation to be
used in production (swap out the Map-based implementation used by the functions
that reference rateLimitStore and the cleanup interval with a RedisStore
implementation).
- Around line 140-148: The cf-connecting-ip and true-client-ip headers are
currently trusted unconditionally; update the logic around where cfConnectingIp
and trueClientIp are read (the block that calls isPublicIp(...) and
normalizeIp(...)) to only honor those headers when the request is coming from a
known/trusted proxy or when a platform flag is set: either (a) require the
immediate peer (remote address) to be in TRUSTED_PROXIES before reading
cf-connecting-ip/true-client-ip, or (b) gate acceptance behind a config flag
like TRUSTED_PLATFORM==='cloudflare'; mirror the existing treatment used for
x-forwarded-for so you reuse the same trusted-proxy checks and ensure
isPublicIp/normalizeIp are only applied after that verification.
- Around line 139-175: getClientIp currently returns the literal "unknown" which
collapses all unidentified clients into one rate-limit bucket; change
getClientIp to return null/undefined when it cannot determine a public client IP
(preserve use of normalizeIp and firstValidPublicIp) and remove the "unknown"
fallback, then update rateLimitRequest to detect a missing client IP and instead
key rate limits by a secondary identifier (preferably Authorization bearer token
or a session cookie name used by your app, falling back to a per-request
fingerprint like a hashed User-Agent+Accept-Language+IP-less-id) so each
user/session gets a distinct bucket; additionally add a startup check that logs
a warning if TRUSTED_PROXIES is empty and no platform headers (cf-connecting-ip
/ true-client-ip / x-real-ip) are configured so operators are alerted.
- Around line 116-122: The IPv6 link-local check currently only matches "fe80:";
update the check on the local variable lower so it matches the entire fe80::/10
range (fe80 through febf). Replace the single startsWith("fe80:") test with a
range test that confirms the first two hex bytes are between 0xfe80 and 0xfebf
(e.g., by validating lower begins with "fe" and the next hex byte is between
0x80 and 0xbf or by using a regex for /^fe[89ab][0-9a-f]:/i), keeping the other
conditions (\"::1\", \"fc\", \"fd\") intact.
In `@src/proxy.ts`:
- Around line 53-60: The current logic sets responseOrigin = allowedOrigins[0]
when origin is missing/invalid which causes the code to send an incorrect
Access-Control-Allow-Origin and still set Access-Control-Allow-Credentials;
update the handling in the proxy so that responseOrigin is only set when
isValidOrigin(origin, allowedOrigins) returns true or
allowedOrigins.includes("*") is true, and otherwise do not emit the
Access-Control-Allow-Origin header nor the Access-Control-Allow-Credentials
header; adjust the code paths around the responseOrigin variable (and any places
that set headers using responseOrigin, e.g., where
Access-Control-Allow-Credentials is set) so headers are only added when a valid
responseOrigin or "*" was determined and ensure Vary: Origin is only added when
Access-Control-Allow-Origin is sent.
- Around line 94-120: The 429 rate-limit response created in the if
(!rateLimitResult.allowed) block does not include the CORS headers assembled in
corsHeaders, so cross-origin clients cannot read the response body or
Retry-After/X-RateLimit headers; update the NextResponse.json call to merge
corsHeaders into the response headers (combine corsHeaders with the existing
headers object, preserving Retry-After, X-RateLimit-Remaining,
X-RateLimit-Reset) so the returned response exposes the throttling info to
browsers; reference the rateLimitResult check and the NextResponse.json call
when making this change.
---
Outside diff comments:
In `@src/components/chat-input.tsx`:
- Around line 101-155: The fetchUsersAndRoles function can race when
mentionQuery changes; create an AbortController for each call (or separate
controllers for the user fetch and the roles fetch), pass controller.signal to
both fetch calls, store the controller so the debounce cleanup can call
controller.abort(), and in the fetch promise chains catch AbortError and skip
calling setAvailableUsers/setMentionableRoles; additionally, before calling the
setters verify the request wasn't aborted (e.g., check controller.signal.aborted
or compare the current mentionQuery/serverId) so stale responses don't overwrite
fresh state; ensure the finally block still sets setIsLoadingUsers(false) only
for non-aborted flows or handles aborted flows appropriately.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ad8707f6-e070-49e2-bad3-3b5296be709f
📒 Files selected for processing (18)
src/__tests__/api-routes/servers-public.test.tssrc/__tests__/middleware.test.tssrc/__tests__/rate-limit-env.test.tssrc/__tests__/realtime-pool.test.tssrc/app/api/servers/[serverId]/mentionable-roles/route.tssrc/app/api/servers/public/route.tssrc/app/invite/[code]/InvitePreviewClient.tsxsrc/app/layout.tsxsrc/app/page.tsxsrc/components/app-layout.tsxsrc/components/chat-input.tsxsrc/components/header.tsxsrc/components/notifications-menu.tsxsrc/contexts/auth-context.tsxsrc/lib/auth-server.tssrc/lib/rate-limit.tssrc/lib/realtime-pool.tssrc/proxy.ts
| resetAt: number; | ||
| } | ||
|
|
||
| const rateLimitStore = new Map<string, RateLimitEntry>(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift
In-memory store won't survive multi-instance / serverless deployments.
rateLimitStore is a per-process Map, and the cleanup interval runs per process. In any deployment with >1 worker (PM2, multi-pod, serverless invocations), each instance enforces its own limit, so a 10/min limit effectively becomes 10 * N per minute, and serverless cold starts reset entirely. This is fine as a baseline, but please document it and plan a Redis (or other shared store) backend before relying on these limits for security-critical throttling (e.g., login attempts).
Also applies to: 177-190
🤖 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/lib/rate-limit.ts` at line 8, The current in-memory Map rateLimitStore
(and its cleanup interval) is per-process and won’t work correctly in
multi-instance or serverless deployments; update the code to (1) add a clear
comment/docstring near rateLimitStore and the cleanup logic explaining this
limitation and that it’s only suitable for single-process/local use, and (2)
refactor the rate limiting to use an interchangeable store interface (e.g.,
IRateLimitStore with methods like get/set/delete/cleanup) and provide a
Redis-backed implementation to be used in production (swap out the Map-based
implementation used by the functions that reference rateLimitStore and the
cleanup interval with a RedisStore implementation).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/proxy.ts (1)
190-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject protocol-relative redirect targets.
Line 191 accepts any value starting with
/, including//evil.example.new URL(destination, request.url)will treat that as an external URL, so this reopens an open-redirect on the login/register flow.🔒 Proposed fix
- const destination = redirect?.startsWith("/") ? redirect : "/"; + const destination = + redirect && /^\/(?!\/)/.test(redirect) ? redirect : "/"; return NextResponse.redirect(new URL(destination, request.url));🤖 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/proxy.ts` around lines 190 - 192, The redirect handling accepts protocol-relative targets like "//evil.example"; update the check for redirect (from request.nextUrl.searchParams.get("redirect")) to only allow single-slash absolute paths by rejecting values that start with "//" or match a protocol (e.g., "http:"), e.g. require redirect to match a pattern like /^\/(?!\/)/ before setting destination and calling NextResponse.redirect(new URL(destination, request.url)); if the check fails, fall back to "/" to prevent open redirects.
♻️ Duplicate comments (1)
src/lib/rate-limit.ts (1)
159-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't derive proxy trust from
X-Forwarded-For.Line 176 treats the last
x-forwarded-forhop as the immediate peer, but that header is still client-controlled at this point. A caller can append a trusted proxy IP there, makeisTrustedProxyRequest()return true, and then spoof the downstream client-IP headers to evade or shard throttling.🛡️ Minimal safe fix
function getImmediatePeerIp(request: Request): string | null { const requestWithIp = request as Request & { ip?: string | null }; const requestIp = requestWithIp.ip; if (typeof requestIp === "string" && requestIp.trim().length > 0) { return normalizeIp(requestIp); } - - const forwardedFor = request.headers.get("x-forwarded-for"); - if (!forwardedFor) { - return null; - } - - const ips = forwardedFor - .split(",") - .map((ip) => ip.trim()) - .filter(Boolean); - - const proxyIp = ips.at(-1); - if (!proxyIp) { - return null; - } - - return normalizeIp(proxyIp); + + // Never establish proxy trust from X-Forwarded-For itself. + return null; }Also applies to: 184-190
🤖 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/lib/rate-limit.ts` around lines 159 - 181, getImmediatePeerIp is currently treating the last hop in X-Forwarded-For as the immediate peer even when the request has not been validated as coming from a trusted proxy; update getImmediatePeerIp to never derive trust from X-Forwarded-For unless the request is from a trusted proxy by checking isTrustedProxyRequest(request) before reading or using the x-forwarded-for header, and only then use normalizeIp on the appropriate hop; also make the same change to the other X-Forwarded-For usage noted (lines 184-190) so both places require isTrustedProxyRequest(request) before trusting or parsing the header.
🤖 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/app/api/servers/`[serverId]/mentionable-roles/route.ts:
- Around line 152-175: The current code in route.ts uses
getAllDocumentsPaginated to compute memberCountByRoleId by scanning all
roleAssignments (mentionableAssignments), which is too expensive on the hot
mention autocomplete path; remove the paginated scan and instead read a
precomputed member count from role metadata or an aggregate source (e.g., a
memberCount field on the Role document) for the mentionableRoleIds, or simply
omit member counts from this endpoint; update any uses of memberCountByRoleId
accordingly and eliminate the getAllDocumentsPaginated call and the for-loops
that build memberCountByRoleId so the endpoint no longer performs a full
roleAssignments scan.
In `@src/components/chat-input.tsx`:
- Around line 109-195: The outer try/catch currently wraps both the users fetch
and the roles fetch so a failure in `/api/users/search` prevents the
`/mentionable-roles` logic from running; split these into two independent flows
(either two separate try/catch blocks or use Promise.allSettled) so failures
don’t cancel the other source: keep the users fetch block that calls
`/api/users/search` and updates setAvailableUsers in its own try/catch (honoring
controller.signal.aborted and handling AbortError), and move the server-specific
logic that reads/sets mentionableRolesCacheRef.current[serverId], fetches
`/api/servers/${serverId}/mentionable-roles`, filters into typedRoles, and calls
setMentionableRoles into a separate try/catch that also checks
controller.signal.aborted and handles AbortError; ensure each catch only clears
its own state (setAvailableUsers or setMentionableRoles) and does not suppress
the other.
---
Outside diff comments:
In `@src/proxy.ts`:
- Around line 190-192: The redirect handling accepts protocol-relative targets
like "//evil.example"; update the check for redirect (from
request.nextUrl.searchParams.get("redirect")) to only allow single-slash
absolute paths by rejecting values that start with "//" or match a protocol
(e.g., "http:"), e.g. require redirect to match a pattern like /^\/(?!\/)/
before setting destination and calling NextResponse.redirect(new
URL(destination, request.url)); if the check fails, fall back to "/" to prevent
open redirects.
---
Duplicate comments:
In `@src/lib/rate-limit.ts`:
- Around line 159-181: getImmediatePeerIp is currently treating the last hop in
X-Forwarded-For as the immediate peer even when the request has not been
validated as coming from a trusted proxy; update getImmediatePeerIp to never
derive trust from X-Forwarded-For unless the request is from a trusted proxy by
checking isTrustedProxyRequest(request) before reading or using the
x-forwarded-for header, and only then use normalizeIp on the appropriate hop;
also make the same change to the other X-Forwarded-For usage noted (lines
184-190) so both places require isTrustedProxyRequest(request) before trusting
or parsing the header.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0141d6e6-043c-438b-a881-87cfca2f0c40
⛔ Files ignored due to path filters (1)
.gitignoreis excluded by none and included by none
📒 Files selected for processing (7)
src/__tests__/rate-limit-env.test.tssrc/app/api/servers/[serverId]/mentionable-roles/route.tssrc/app/invite/[code]/InvitePreviewClient.tsxsrc/components/chat-input.tsxsrc/lib/auth-server.tssrc/lib/rate-limit.tssrc/proxy.ts
Owner
Author
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.
This pull request introduces several improvements and updates across the codebase, focusing on configuration flexibility, dependency updates, test reliability, and minor UI accessibility enhancements. The most significant changes are grouped below:
Configuration and Environment Variables:
.env.local.examplewith new environment variables for mobile app support, rate limiting, and CORS configuration, making it easier to customize deployments and enhance security.Dependency Updates:
package.json(including@tanstack/react-query,posthog-js,posthog-node,yaml, and several@typesand linting packages) to their latest versions for improved stability and compatibility. Also added new scripts for dependency management. [1] [2] [3] [4] [5]Testing Reliability and Consistency:
.mockResolvedValue()instead of custom async functions with explicitundefinedreturns, ensuring more consistent and modern mocking patterns. [1] [2] [3] [4] [5] [6] [7] [8] [9]/api/servers/publicto use a properNextRequestobject, improving test accuracy and alignment with actual usage. [1] [2] [3] [4] [5] [6] [7] [8]UI and Accessibility Improvements:
aria-hidden="true"to decorative icons and simplified conditional rendering for cleaner code. [1] [2] [3] [4]Code Quality:
These changes collectively enhance maintainability, security, test reliability, and user experience.