Continue to build out mobile app & implment new appwrite presences API#191
Open
acarlson33 wants to merge 13 commits into
Conversation
FirepitProject ID: Sites (2)
Tip Realtime gives you live updates for users, storage, functions, and databases |
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:
📝 WalkthroughWalkthroughAdds mobile messaging: splits server screen into a browser and a new message screen, introduces message listing and server fetch APIs, expands message types (reactions/thread/local), wires new tab-driven navigation and tab screens, updates bootstrap auth to use session secrets, and includes build/config and dependency edits. ChangesServer Messaging Feature
Mobile UI and Tabs
Bootstrap and Auth
Build / Config / Packages
🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 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)
✨ Simplify code
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}
Owner
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@apps/mobile/src/app/explore.tsx`:
- Around line 119-125: The current filter in discoverablePublicServers lets
servers with no $id through, but onOpen and onJoin bail out when $id is missing;
change the predicate to only include servers that have a valid $id and are not
in joinedServerIds (i.e., require server.$id &&
!joinedServerIds.has(server.$id)) so UI and actions remain consistent; update
any related logic that assumes missing $id (refer to discoverablePublicServers,
publicServers, joinedServerIds, and the onOpen/onJoin handlers) to ensure
servers without $id are either excluded or handled explicitly.
In `@apps/mobile/src/app/server/`[serverId].tsx:
- Around line 207-213: The ThemedText component instance that calls
router.push("/explore") (and similar at lines referenced) is missing
accessibilityRole and keyboard handlers; wrap the clickable text in a Pressable
(or ensure ThemedText adds these) and set accessibilityRole="link" or "button",
add at least one keyboard handler (onKeyDown or onKeyUp) that triggers the same
router.push navigation, and preserve the existing onPress handler so screen
readers and keyboard users can activate the navigation; update both the
ThemedText usage at this spot and the other occurrence (around the 287-293
range).
- Line 127: The style array on the ScrollView has the same entry twice
(styles.scrollView used twice); remove the duplicate so the style prop is either
a single reference (style={styles.scrollView} or style={[styles.scrollView]}) or
replace the second entry with the intended style (e.g. styles.container /
styles.content) if a different style was meant—search for the ScrollView
component with style={[styles.scrollView, styles.scrollView]} and update it
and/or verify the styles object to ensure the correct secondary style is
applied.
In `@apps/mobile/src/app/server/messages/`[serverId]/[channelId].tsx:
- Around line 277-291: The ThemedText element with an onPress handler should be
made accessible: update the ThemedText (inside the ThemedView) to include
accessibilityRole="button" and accessible={true}, provide a clear
accessibilityLabel if needed, and add keyboard support by implementing an
onKeyPress (or onKeyDown depending on platform) handler that triggers the same
navigation logic (check normalizedServerId then call
router.replace(`/server/${normalizedServerId}`)) when Enter or Space is pressed;
ensure the existing onPress logic is reused to avoid duplication.
- Around line 391-474: The FlatList here is nested inside a parent ScrollView
causing scroll conflicts (see FlatList component with nestedScrollEnabled and
style={styles.messageList}); remove the nested scrolling by making the message
list the primary scroll container: move any surrounding ScrollView content into
FlatList's ListHeaderComponent / ListFooterComponent or convert the screen to
use a SectionList that combines header/content and messages, and then remove
nestedScrollEnabled from the FlatList; update any layout/styles (e.g.,
styles.timelineList, styles.messageList) and ensure MessageCard and
MessageWithMentions still receive the same props when rendered by the top-level
list.
- Around line 420-423: The fallback using (message as any).id bypasses type
safety; update the Message type (or the type alias used for the variable
`message`) to include an optional `id` property so you can safely read
`message.id` as a fallback when `$id` is absent, and then set `msgId` using
`message.$id ?? message.id`. Alternatively, add a narrow type guard function
(e.g., isMessageWithId(message): message is MessageWithId) and use it before
accessing `message.id`; avoid using `any` and ensure `msgId` assignment in the
`msgId` variable uses the new typed property or guard.
- Around line 701-709: The Pressable wrapping the image needs accessibility
props so screen readers can interact with it: update the Pressable (the
component using onOpenImageViewer and imageUrl) to include
accessibilityRole="button" and a descriptive accessibilityLabel (e.g., "Open
image viewer" or "Open image preview" — or derive a more specific label from
image metadata if available) so users can understand and activate the image;
ensure these props are added to the Pressable element that contains the Image.
In `@apps/mobile/src/lib/firepit/messages.ts`:
- Around line 33-46: The helper fetchChannelMessages currently only requests the
first page; add an optional cursorAfter parameter to its signature (e.g.,
fetchChannelMessages(..., limit = 50, cursorAfter?: string)) and include it in
the firepitRequest query when present so subsequent pages can be fetched using
the returned nextCursor/ cursorAfter token from MessageListResponse; update the
function parameter list and the query object to conditionally pass cursorAfter.
In `@apps/web/src/app/api/messages/route.ts`:
- Around line 329-333: The required-field validation is using the raw text
variable `text` which allows whitespace-only or non-string truthy inputs to
pass; change validations that currently reference `text` (the check around where
the message is persisted) to use the already-normalized `normalizedText` instead
(keep `const normalizedText = typeof text === "string" ? text : "";` and use
`normalizedText` for the required-field checks and any emptiness/length checks),
and ensure logic that computes `creatingPoll` and `validMentions` still
references `normalizedText` where appropriate so whitespace-only input cannot be
saved as a non-empty message.
- Around line 194-201: The handler always returns nextCursor: null which breaks
pagination; update the return to compute and return a real cursor by using the
backend response or the fetched documents: after mapping response.documents with
mapMessageDocument, set nextCursor to response.nextCursor if present, otherwise
derive it from the last mapped message (e.g., its id or createdAt) only when the
number of items equals the requested limit (using cursorAfter and limit), and
return that value in the ListMessagesResponse instead of null so cursor
pagination works correctly.
🪄 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: aa1f3729-8564-4475-a0c4-e2f63cbab0b0
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lockand included bybun.lock
📒 Files selected for processing (10)
apps/mobile/src/app/_layout.tsxapps/mobile/src/app/explore.tsxapps/mobile/src/app/server/[serverId].tsxapps/mobile/src/app/server/messages/[serverId]/[channelId].tsxapps/mobile/src/lib/firepit/messages.tsapps/mobile/src/lib/firepit/servers.tsapps/mobile/src/lib/firepit/types.tsapps/web/package.jsonapps/web/src/app/api/messages/route.tsapps/web/src/app/api/servers/[serverId]/route.ts
Comment on lines
+119
to
+125
| const discoverablePublicServers = useMemo( | ||
| () => | ||
| publicServers.filter( | ||
| (server) => !server.$id || !joinedServerIds.has(server.$id), | ||
| ), | ||
| [joinedServerIds, publicServers], | ||
| ); |
Contributor
There was a problem hiding this comment.
Verify intent: servers without $id always pass the filter.
The condition !server.$id || !joinedServerIds.has(server.$id) means any server lacking an $id will always appear in discoverablePublicServers. If the backend can return servers without IDs, consider whether they should be displayed (since onOpen and onJoin both early-return when $id is missing).
() =>
publicServers.filter(
- (server) => !server.$id || !joinedServerIds.has(server.$id),
+ (server) => server.$id && !joinedServerIds.has(server.$id),
),🤖 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 `@apps/mobile/src/app/explore.tsx` around lines 119 - 125, The current filter
in discoverablePublicServers lets servers with no $id through, but onOpen and
onJoin bail out when $id is missing; change the predicate to only include
servers that have a valid $id and are not in joinedServerIds (i.e., require
server.$id && !joinedServerIds.has(server.$id)) so UI and actions remain
consistent; update any related logic that assumes missing $id (refer to
discoverablePublicServers, publicServers, joinedServerIds, and the onOpen/onJoin
handlers) to ensure servers without $id are either excluded or handled
explicitly.
| return ( | ||
| <AuthRouteGuard> | ||
| <ScrollView | ||
| style={[styles.scrollView, styles.scrollView]} |
Contributor
There was a problem hiding this comment.
Duplicate style reference.
styles.scrollView is applied twice in the style array, which appears to be a copy-paste error.
<ScrollView
- style={[styles.scrollView, styles.scrollView]}
+ style={styles.scrollView}
contentContainerStyle={styles.scrollContent}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| style={[styles.scrollView, styles.scrollView]} | |
| style={styles.scrollView} | |
| contentContainerStyle={styles.scrollContent} |
🤖 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 `@apps/mobile/src/app/server/`[serverId].tsx at line 127, The style array on
the ScrollView has the same entry twice (styles.scrollView used twice); remove
the duplicate so the style prop is either a single reference
(style={styles.scrollView} or style={[styles.scrollView]}) or replace the second
entry with the intended style (e.g. styles.container / styles.content) if a
different style was meant—search for the ScrollView component with
style={[styles.scrollView, styles.scrollView]} and update it and/or verify the
styles object to ensure the correct secondary style is applied.
Comment on lines
+207
to
+213
| <ThemedText | ||
| type="smallBold" | ||
| onPress={() => router.push("/explore")} | ||
| > | ||
| Back to server browser | ||
| </ThemedText> | ||
| </ThemedView> |
Contributor
There was a problem hiding this comment.
Add accessibility role and keyboard support to pressable text elements.
ThemedText with onPress lacks accessibilityRole and keyboard event handlers. Screen reader users and keyboard-only users cannot interact with these navigation links properly.
Consider wrapping in a Pressable with accessibilityRole="link" or "button", or ensure ThemedText internally handles accessibility when onPress is provided.
As per coding guidelines: "Accompany onClick with at least one of: onKeyUp, onKeyDown, or onKeyPress" and "Make static elements with click handlers use a valid role attribute".
Also applies to: 287-293
🤖 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 `@apps/mobile/src/app/server/`[serverId].tsx around lines 207 - 213, The
ThemedText component instance that calls router.push("/explore") (and similar at
lines referenced) is missing accessibilityRole and keyboard handlers; wrap the
clickable text in a Pressable (or ensure ThemedText adds these) and set
accessibilityRole="link" or "button", add at least one keyboard handler
(onKeyDown or onKeyUp) that triggers the same router.push navigation, and
preserve the existing onPress handler so screen readers and keyboard users can
activate the navigation; update both the ThemedText usage at this spot and the
other occurrence (around the 287-293 range).
Comment on lines
+277
to
+291
| <ThemedText | ||
| type="smallBold" | ||
| onPress={() => { | ||
| if (!normalizedServerId) { | ||
| return; | ||
| } | ||
|
|
||
| router.replace( | ||
| `/server/${normalizedServerId}`, | ||
| ); | ||
| }} | ||
| > | ||
| Back to server browser | ||
| </ThemedText> | ||
| </ThemedView> |
Contributor
There was a problem hiding this comment.
Add accessibility role and keyboard support to pressable text.
Same issue as the server browser screen: ThemedText with onPress lacks proper accessibility support for screen readers and keyboard navigation.
As per coding guidelines: "Make static elements with click handlers use a valid role attribute".
🤖 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 `@apps/mobile/src/app/server/messages/`[serverId]/[channelId].tsx around lines
277 - 291, The ThemedText element with an onPress handler should be made
accessible: update the ThemedText (inside the ThemedView) to include
accessibilityRole="button" and accessible={true}, provide a clear
accessibilityLabel if needed, and add keyboard support by implementing an
onKeyPress (or onKeyDown depending on platform) handler that triggers the same
navigation logic (check normalizedServerId then call
router.replace(`/server/${normalizedServerId}`)) when Enter or Space is pressed;
ensure the existing onPress logic is reused to avoid duplication.
Comment on lines
+391
to
+474
| <FlatList | ||
| data={messages} | ||
| inverted | ||
| keyExtractor={(message, index) => | ||
| message.$id ?? | ||
| `${message.channelId}-${message.$createdAt}-${index}` | ||
| } | ||
| renderItem={({ item: message }) => ( | ||
| <MessageCard | ||
| message={message} | ||
| isMine={Boolean( | ||
| message.userId && | ||
| currentUser && | ||
| message.userId === | ||
| (currentUser.$id ?? | ||
| currentUser.userId), | ||
| )} | ||
| onOpenImageViewer={( | ||
| url?: string, | ||
| ) => | ||
| setViewerImageUrl( | ||
| url ?? null, | ||
| ) | ||
| } | ||
| onToggleReaction={async ( | ||
| emoji: string, | ||
| isAdding: boolean, | ||
| ) => { | ||
| try { | ||
| const msgId = | ||
| message.$id ?? | ||
| (message as any) | ||
| .id; | ||
| if (!msgId) { | ||
| return; | ||
| } | ||
|
|
||
| await toggleReaction( | ||
| String(msgId), | ||
| emoji, | ||
| isAdding, | ||
| false, | ||
| ); | ||
| } catch { | ||
| setMessageError( | ||
| "Unable to update reaction", | ||
| ); | ||
| } | ||
| }} | ||
| > | ||
| <MessageWithMentions | ||
| text={ | ||
| message.text ?? "" | ||
| } | ||
| /> | ||
| </MessageCard> | ||
| )} | ||
| ListEmptyComponent={ | ||
| <ThemedView | ||
| type="secondary" | ||
| style={styles.emptyTimeline} | ||
| > | ||
| <ThemedText type="smallBold"> | ||
| No messages yet | ||
| </ThemedText> | ||
| <ThemedText | ||
| themeColor="mutedForeground" | ||
| style={styles.copy} | ||
| > | ||
| Send the first message to | ||
| start the conversation. | ||
| </ThemedText> | ||
| </ThemedView> | ||
| } | ||
| initialNumToRender={10} | ||
| maxToRenderPerBatch={10} | ||
| windowSize={5} | ||
| nestedScrollEnabled | ||
| showsVerticalScrollIndicator | ||
| contentContainerStyle={ | ||
| styles.timelineList | ||
| } | ||
| style={styles.messageList} | ||
| /> |
Contributor
There was a problem hiding this comment.
FlatList nested inside ScrollView causes scroll conflicts.
A FlatList with nestedScrollEnabled inside a parent ScrollView creates competing scroll containers. On iOS this can cause janky scrolling and missed gestures. On Android, nestedScrollEnabled helps but still degrades UX.
Consider restructuring so the message timeline is the primary scroll container, or use a SectionList for the entire screen instead of nesting.
🤖 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 `@apps/mobile/src/app/server/messages/`[serverId]/[channelId].tsx around lines
391 - 474, The FlatList here is nested inside a parent ScrollView causing scroll
conflicts (see FlatList component with nestedScrollEnabled and
style={styles.messageList}); remove the nested scrolling by making the message
list the primary scroll container: move any surrounding ScrollView content into
FlatList's ListHeaderComponent / ListFooterComponent or convert the screen to
use a SectionList that combines header/content and messages, and then remove
nestedScrollEnabled from the FlatList; update any layout/styles (e.g.,
styles.timelineList, styles.messageList) and ensure MessageCard and
MessageWithMentions still receive the same props when rendered by the top-level
list.
Comment on lines
+420
to
+423
| const msgId = | ||
| message.$id ?? | ||
| (message as any) | ||
| .id; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Avoid as any type assertion.
The fallback (message as any).id bypasses type safety. If Message can have an id property in addition to $id, update the type definition instead of using any.
const msgId =
message.$id ??
- (message as any)
- .id;
+ message.id;As per coding guidelines: "Don't use the any type".
🤖 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 `@apps/mobile/src/app/server/messages/`[serverId]/[channelId].tsx around lines
420 - 423, The fallback using (message as any).id bypasses type safety; update
the Message type (or the type alias used for the variable `message`) to include
an optional `id` property so you can safely read `message.id` as a fallback when
`$id` is absent, and then set `msgId` using `message.$id ?? message.id`.
Alternatively, add a narrow type guard function (e.g., isMessageWithId(message):
message is MessageWithId) and use it before accessing `message.id`; avoid using
`any` and ensure `msgId` assignment in the `msgId` variable uses the new typed
property or guard.
Comment on lines
+701
to
+709
| <Pressable | ||
| onPress={() => onOpenImageViewer?.(imageUrl)} | ||
| style={{ marginTop: Spacing.one }} | ||
| > | ||
| <Image | ||
| source={{ uri: imageUrl }} | ||
| style={{ width: 200, height: 120, borderRadius: 8 }} | ||
| /> | ||
| </Pressable> |
Contributor
There was a problem hiding this comment.
Add accessibility attributes to image pressable.
The Pressable wrapping the message image lacks accessibilityRole and accessibilityLabel, making it inaccessible to screen reader users.
{imageUrl ? (
<Pressable
onPress={() => onOpenImageViewer?.(imageUrl)}
+ accessibilityRole="button"
+ accessibilityLabel="View full image"
style={{ marginTop: Spacing.one }}
>As per coding guidelines: "Give all elements requiring alt text meaningful information for screen readers".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Pressable | |
| onPress={() => onOpenImageViewer?.(imageUrl)} | |
| style={{ marginTop: Spacing.one }} | |
| > | |
| <Image | |
| source={{ uri: imageUrl }} | |
| style={{ width: 200, height: 120, borderRadius: 8 }} | |
| /> | |
| </Pressable> | |
| <Pressable | |
| onPress={() => onOpenImageViewer?.(imageUrl)} | |
| accessibilityRole="button" | |
| accessibilityLabel="View full image" | |
| style={{ marginTop: Spacing.one }} | |
| > | |
| <Image | |
| source={{ uri: imageUrl }} | |
| style={{ width: 200, height: 120, borderRadius: 8 }} | |
| /> | |
| </Pressable> |
🤖 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 `@apps/mobile/src/app/server/messages/`[serverId]/[channelId].tsx around lines
701 - 709, The Pressable wrapping the image needs accessibility props so screen
readers can interact with it: update the Pressable (the component using
onOpenImageViewer and imageUrl) to include accessibilityRole="button" and a
descriptive accessibilityLabel (e.g., "Open image viewer" or "Open image
preview" — or derive a more specific label from image metadata if available) so
users can understand and activate the image; ensure these props are added to the
Pressable element that contains the Image.
Comment on lines
+33
to
+46
| export async function fetchChannelMessages( | ||
| baseUrl: string, | ||
| token: string, | ||
| channelId: string, | ||
| limit = 50, | ||
| ) { | ||
| return firepitRequest<MessageListResponse>({ | ||
| baseUrl, | ||
| path: "/api/messages", | ||
| token, | ||
| query: { | ||
| channelId, | ||
| limit, | ||
| }, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Expose cursorAfter in fetchChannelMessages to support paging.
Line 33-Line 46 only allow first-page reads (channelId + limit). Once nextCursor is returned, this helper cannot request subsequent pages.
Proposed diff
export async function fetchChannelMessages(
baseUrl: string,
token: string,
channelId: string,
limit = 50,
+ cursorAfter?: string,
) {
return firepitRequest<MessageListResponse>({
baseUrl,
path: "/api/messages",
token,
query: {
channelId,
limit,
+ ...(cursorAfter ? { cursorAfter } : {}),
},
});
}🤖 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 `@apps/mobile/src/lib/firepit/messages.ts` around lines 33 - 46, The helper
fetchChannelMessages currently only requests the first page; add an optional
cursorAfter parameter to its signature (e.g., fetchChannelMessages(..., limit =
50, cursorAfter?: string)) and include it in the firepitRequest query when
present so subsequent pages can be fetched using the returned nextCursor/
cursorAfter token from MessageListResponse; update the function parameter list
and the query object to conditionally pass cursorAfter.
Comment on lines
+194
to
+201
| const messages = (response.documents ?? []).map((doc) => | ||
| mapMessageDocument(doc as Record<string, unknown>), | ||
| ); | ||
|
|
||
| return NextResponse.json<ListMessagesResponse>({ | ||
| messages, | ||
| nextCursor: null, | ||
| }); |
Contributor
There was a problem hiding this comment.
Return a real nextCursor instead of always null.
Line 198-Line 200 always return nextCursor: null, which breaks cursor pagination despite accepting cursorAfter and limit.
Proposed diff
- const messages = (response.documents ?? []).map((doc) =>
+ const documents = response.documents ?? [];
+ const messages = documents.map((doc) =>
mapMessageDocument(doc as Record<string, unknown>),
);
+ const lastDocument = documents.at(-1) as { $id?: unknown } | undefined;
+ const nextCursor =
+ documents.length === limit && typeof lastDocument?.$id === "string"
+ ? lastDocument.$id
+ : null;
return NextResponse.json<ListMessagesResponse>({
messages,
- nextCursor: null,
+ nextCursor,
});🤖 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 `@apps/web/src/app/api/messages/route.ts` around lines 194 - 201, The handler
always returns nextCursor: null which breaks pagination; update the return to
compute and return a real cursor by using the backend response or the fetched
documents: after mapping response.documents with mapMessageDocument, set
nextCursor to response.nextCursor if present, otherwise derive it from the last
mapped message (e.g., its id or createdAt) only when the number of items equals
the requested limit (using cursorAfter and limit), and return that value in the
ListMessagesResponse instead of null so cursor pagination works correctly.
Comment on lines
329
to
+333
| const normalizedText = typeof text === "string" ? text : ""; | ||
| const creatingPoll = isPollCommand(normalizedText); | ||
| const validMentions = !creatingPoll ? normalizeMentionIds(mentions) : []; | ||
| const validMentions = !creatingPoll | ||
| ? normalizeMentionIds(mentions) | ||
| : []; |
Contributor
There was a problem hiding this comment.
Use normalized text content for required-field validation.
Line 375 checks raw text, so whitespace-only or non-string truthy inputs can pass validation and still be persisted as empty message text.
Proposed diff
- const normalizedText = typeof text === "string" ? text : "";
+ const normalizedText = typeof text === "string" ? text : "";
+ const hasTextContent = normalizedText.trim().length > 0;
const creatingPoll = isPollCommand(normalizedText);
@@
- if (
- (!text && !imageFileId && normalizedAttachments.length === 0) ||
- !channelId
- ) {
+ if (
+ (!hasTextContent &&
+ !imageFileId &&
+ normalizedAttachments.length === 0) ||
+ !channelId
+ ) {
return NextResponse.json(
{
error: "text, imageFileId, or attachments, and channelId are required",
},
{ status: 400 },
);
}
@@
- text: parsedPoll ? parsedPoll.question : normalizedText || "",
+ text: parsedPoll
+ ? parsedPoll.question
+ : hasTextContent
+ ? normalizedText
+ : "",Also applies to: 375-377
🤖 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 `@apps/web/src/app/api/messages/route.ts` around lines 329 - 333, The
required-field validation is using the raw text variable `text` which allows
whitespace-only or non-string truthy inputs to pass; change validations that
currently reference `text` (the check around where the message is persisted) to
use the already-normalized `normalizedText` instead (keep `const normalizedText
= typeof text === "string" ? text : "";` and use `normalizedText` for the
required-field checks and any emptiness/length checks), and ensure logic that
computes `creatingPoll` and `validMentions` still references `normalizedText`
where appropriate so whitespace-only input cannot be saved as a non-empty
message.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@apps/mobile/app.json`:
- Around line 14-23: The manifest shows Android config but the Firebase config
was removed; confirm whether Android remote push is required and either restore
the Firebase config (add back android.googleServicesFile / provide
google-services.json used by expo-notifications) or remove the push plugin:
locate the apps/mobile app.json's "android" block and the project configuration
that references expo-notifications, and if keeping push support re-add
android.googleServicesFile pointing to the proper google-services.json or
document where the JSON is provided at build time; if not keeping push support,
remove the expo-notifications plugin and related usage to avoid a broken/unused
configuration.
In `@apps/mobile/eas.json`:
- Line 12: Replace the user-specific absolute TMPDIR value
("/home/august/eas-build-tmp/") in the eas.json entry for "TMPDIR" with a
portable repo-relative path or remove the override entirely; update the "TMPDIR"
key to use the same repo-relative pattern used for iOS (e.g., a relative
./eas-build-tmp or project-root temp folder) so CI and other developer machines
can resolve it reliably instead of hardcoding the username-specific path.
In `@apps/mobile/scripts/build-dev.ts`:
- Line 9: In apps/mobile/scripts/build-dev.ts replace all usages of console.log
and console.error (e.g., the "Building for iOS and Android on macOS..." message
and the other console calls) with the approved output API: use
process.stdout.write for informational messages and process.stderr.write for
errors, or call the repository logging helper if one exists; update every
occurrence of console.log/console.error in that file so message formatting and
newlines are preserved when converting to
process.stdout.write/process.stderr.write.
In `@apps/mobile/scripts/build.ts`:
- Around line 9-10: Replace all console.* calls in the mobile build scripts with
process.stdout.write for normal messages and process.stderr.write for errors: in
build.ts and build-dev.ts find occurrences of console.log and console.error (the
messages surrounding the execSync("bunx eas-cli build --platform ios --local",
...) call and similar execSync calls) and change them to
process.stdout.write("...\\n") for informational output and
process.stderr.write("...\\n") for failures; ensure newline characters are
preserved and keep existing message text and surrounding logic unchanged.
In `@apps/mobile/src/app/`(tabs)/chat.tsx:
- Line 91: The onlineCount fallback is incorrectly using channels.length, which
misrepresents online users; change the logic in the onlineCount computation (and
the similar occurrences around lines 597-603) to not use channels.length as the
fallback—use selectedServer?.memberCount ?? 0 (or undefined if callers handle
missing data) or a proper online/member presence metric instead, and update any
downstream uses that expect a numeric count accordingly (refer to the
onlineCount variable, selectedServer.memberCount, and channels usage to locate
the spots to fix).
- Around line 96-102: The effect early-return leaves stale
servers/selectedServerId in memory when workspace/auth becomes unavailable;
update the useEffect(s) that check canLoadWorkspace/instanceUrl/accessToken (the
blocks around the current checks) to explicitly clear state before returning —
call setServers([]) and setSelectedServerId(undefined or null) (use whatever
sentinel the component uses) when !canLoadWorkspace or missing
instanceUrl/accessToken so the UI does not show stale account data; apply the
same change to the later effect that has the same guard (lines ~125-136).
In `@apps/mobile/src/app/index.tsx`:
- Around line 66-81: The current signedIn boolean only checks state and
currentUser, causing premature navigation; update its definition to require both
a valid auth token and the user per the auth-gate contract by adding a token
presence check (e.g., Boolean(bootstrapInstance?.token) or whatever the
bootstrapInstance exposes) so signedIn becomes true only when state === "ready"
&& Boolean(currentUser) && Boolean(bootstrapInstance?.token); adjust references
to signedIn accordingly.
In `@apps/mobile/src/app/login.tsx`:
- Around line 64-77: The signed-in predicate here is inconsistent with
AuthRouteGuard: include accessToken in the bootstrap destructure (add
accessToken from useFirepitBootstrap) and change signedIn to the same check used
by AuthRouteGuard (e.g., state === "ready" && Boolean(currentUser) &&
Boolean(accessToken)) so redirects match guarded routes.
In `@apps/mobile/src/lib/firepit/bootstrap.ts`:
- Around line 316-321: The console.log calls in the auth bootstrap (specifically
inside fetchCurrentUser) must be removed or replaced with the app's logging
abstraction; locate the console.log usage shown in fetchCurrentUser and the
other console calls referenced (near the same auth bootstrap: the blocks at
~356-358 and ~397-400) and either delete them or route the messages through the
established logger (e.g., processLogger/appLogger) at an appropriate level
(debug/info), preserving the original message content format but not using
console.
In `@apps/web/src/__tests__/auth-server.test.ts`:
- Around line 17-22: The tests currently use no-op MockClient.setJWT() and
setSession(), so they don't verify the new branch in getSessionFromHeader();
change the mock implementation to record which setter was called (e.g. set a
property like lastAuthMode = "jwt" or "session" inside MockClient.setJWT and
MockClient.setSession) and update the tests that exercise opaque bearer vs
JWT-shaped tokens (tests around lines 17-22 and 143-173) to assert that
mockClient.lastAuthMode === "session" for opaque tokens and === "jwt" for
JWT-shaped tokens; keep getSessionFromHeader behavior unchanged, only add the
recording and assertions in the tests to validate the correct branch is used.
In `@apps/web/src/lib/auth-server.ts`:
- Around line 53-58: Replace the fragile dot-count heuristic: remove or stop
using isLikelyJwt() inside getSessionFromHeader and instead attempt the session
retrieval flow first (treat bearer as opaque session token) and only call
setJWT() when you have deterministic evidence the token is a JWT (e.g., explicit
token-type flag from the caller or a successful JWT verification/decoding step);
specifically update getSessionFromHeader to try the session lookup using the
bearer token, fall back to JWT handling only after validating the token as a
JWT, and eliminate reliance on isLikelyJwt() to decide the 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: 8277bd7f-8e5f-460c-b2ba-4f066d6480fd
⛔ Files ignored due to path filters (11)
.github/workflows/build:web.ymlis excluded by none and included by none.github/workflows/cleanup-orphan-memberships.ymlis excluded by none and included by none.github/workflows/cleanup-orphan-memberships:web.ymlis excluded by none and included by none.github/workflows/setup-appwrite.ymlis excluded by none and included by none.github/workflows/setup-appwrite:web.ymlis excluded by none and included by none.github/workflows/test:web.ymlis excluded by none and included by none.gitignoreis excluded by none and included by noneapps/mobile/assets/images/tabIcons/home.pngis excluded by!**/*.pngand included byapps/mobile/**apps/mobile/assets/images/tabIcons/home@2x.pngis excluded by!**/*.pngand included byapps/mobile/**apps/mobile/assets/images/tabIcons/home@3x.pngis excluded by!**/*.pngand included byapps/mobile/**bun.lockis excluded by!**/*.lockand included bybun.lock
📒 Files selected for processing (19)
apps/mobile/app.jsonapps/mobile/eas.jsonapps/mobile/package.jsonapps/mobile/scripts/build-dev.tsapps/mobile/scripts/build.tsapps/mobile/src/app/(tabs)/_layout.tsxapps/mobile/src/app/(tabs)/admin.tsxapps/mobile/src/app/(tabs)/chat.tsxapps/mobile/src/app/(tabs)/home.tsxapps/mobile/src/app/(tabs)/settings.tsxapps/mobile/src/app/_layout.tsxapps/mobile/src/app/index.tsxapps/mobile/src/app/login.tsxapps/mobile/src/components/app-tabs.tsxapps/mobile/src/components/app-tabs.web.tsxapps/mobile/src/components/tab-placeholder-screen.tsxapps/mobile/src/lib/firepit/bootstrap.tsapps/web/src/__tests__/auth-server.test.tsapps/web/src/lib/auth-server.ts
| "distribution": "internal", | ||
| "android": { | ||
| "env": { | ||
| "TMPDIR": "/home/august/eas-build-tmp/" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the exact eas.json lines around TMPDIR
if [ -f apps/mobile/eas.json ]; then
echo "== apps/mobile/eas.json (lines 1-60) =="
nl -ba apps/mobile/eas.json | sed -n '1,60p' | cat
else
echo "Missing: apps/mobile/eas.json" >&2
fi
# 2) Search for TMPDIR elsewhere in the repo
echo
echo "== All TMPDIR occurrences =="
rg -n '"TMPDIR"\s*:' -S . || true
echo
rg -n 'TMPDIR' -S . || true
# 3) If eas config references profiles/env, locate likely related keys
echo
echo "== EAS build profile env keys around eas.json =="
rg -n 'build.*profile|profile|env|TMPDIR' -S apps/mobile/eas.json || trueRepository: acarlson33/firepit
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Print eas.json header and the specific line range around 12
if [ -f apps/mobile/eas.json ]; then
echo "== apps/mobile/eas.json (lines 1-40) =="
sed -n '1,40p' apps/mobile/eas.json | cat
echo
echo "== apps/mobile/eas.json (lines 8-18) =="
sed -n '8,18p' apps/mobile/eas.json | cat
else
echo "Missing: apps/mobile/eas.json" >&2
fi
# 2) Search for TMPDIR occurrences across repo (case-sensitive)
echo
echo "== All TMPDIR occurrences (exact key) =="
rg -n '"TMPDIR"\s*:' -S . || true
echo
echo "== All TMPDIR occurrences (any text) =="
rg -n 'TMPDIR' -S . || trueRepository: acarlson33/firepit
Length of output: 1662
Avoid user-specific absolute TMPDIR in apps/mobile/eas.json
apps/mobile/eas.json:12 hardcodes Android TMPDIR to /home/august/eas-build-tmp/, which won’t work on CI/other developer machines. Use the repo-relative path already used for iOS (or remove the override).
Proposed portable change
- "TMPDIR": "/home/august/eas-build-tmp/"
+ "TMPDIR": "./build-tmp/"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "TMPDIR": "/home/august/eas-build-tmp/" | |
| "TMPDIR": "./build-tmp/" |
🤖 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 `@apps/mobile/eas.json` at line 12, Replace the user-specific absolute TMPDIR
value ("/home/august/eas-build-tmp/") in the eas.json entry for "TMPDIR" with a
portable repo-relative path or remove the override entirely; update the "TMPDIR"
key to use the same repo-relative pattern used for iOS (e.g., a relative
./eas-build-tmp or project-root temp folder) so CI and other developer machines
can resolve it reliably instead of hardcoding the username-specific path.
|
|
||
| try { | ||
| if (isMac) { | ||
| console.log("Building for iOS and Android on macOS..."); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the file content around the referenced lines
echo "== apps/mobile/scripts/build-dev.ts =="
ls -la apps/mobile/scripts/build-dev.ts
sed -n '1,120p' apps/mobile/scripts/build-dev.ts
# 2) Confirm all console.* usages in this file
echo "== console usages in build-dev.ts =="
rg -n "console\.(log|error|warn|info|debug)" apps/mobile/scripts/build-dev.ts || true
# 3) Find repo-approved stdout/stderr helpers (common patterns)
echo "== Search for project logging helpers =="
rg -n "stdout\.write|stderr\.write|process\.stdout\.write|process\.stderr\.write" . -S || true
rg -n "logger|log\s*\(" apps/mobile/scripts -S || true
rg -n "no-console" . -S || true
# 4) Check whether this guideline is enforced by lint config (if present)
echo "== ESLint config(s) mentioning no-console =="
rg -n "no-console" .eslintrc* **/.eslintrc* **/eslint* **/*eslint* -S || trueRepository: acarlson33/firepit
Length of output: 4471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for mobile ESLint config =="
fd -a "eslint.config.*|.eslintrc.*|tsconfig.eslint.*" . || true
echo "== Search for no-console setting across repo =="
rg -n "\"no-console\"\s*:\s*\"off\"|no-console" . -S || true
echo "== List likely lint config files =="
ls -la . | sed -n '1,200p'
ls -la apps/mobile || true
ls -la apps/mobile/scripts || true
echo "== Search for eslint configs under apps/ =="
fd "eslint.config.*|.eslintrc.*" apps -a -t f || trueRepository: acarlson33/firepit
Length of output: 2691
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== View eslint.config.js around the no-console rule =="
sed -n '1,140p' apps/web/eslint.config.js
echo "== Search for overrides / root applies patterns =="
rg -n "overrides|files|ignores|rules" apps/web/eslint.config.js || trueRepository: acarlson33/firepit
Length of output: 6438
Replace console.* in apps/mobile/scripts/build-dev.ts with approved stdout/stderr output.
apps/mobile/scripts/build-dev.ts still uses console.log/console.error (lines 9, 23, 31, 33); switch to process.stdout.write / process.stderr.write (or the repo’s logging helper) to follow the “Don’t use console” coding guideline.
🤖 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 `@apps/mobile/scripts/build-dev.ts` at line 9, In
apps/mobile/scripts/build-dev.ts replace all usages of console.log and
console.error (e.g., the "Building for iOS and Android on macOS..." message and
the other console calls) with the approved output API: use process.stdout.write
for informational messages and process.stderr.write for errors, or call the
repository logging helper if one exists; update every occurrence of
console.log/console.error in that file so message formatting and newlines are
preserved when converting to process.stdout.write/process.stderr.write.
Comment on lines
+9
to
+10
| console.log("Building for iOS and Android on macOS..."); | ||
| execSync("bunx eas-cli build --platform ios --local", { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file with line numbers
echo "=== apps/mobile/scripts/build.ts (relevant lines) ==="
if [ -f "apps/mobile/scripts/build.ts" ]; then
nl -ba apps/mobile/scripts/build.ts | sed -n '1,120p'
else
echo "MISSING: apps/mobile/scripts/build.ts"
fi
# Search for console.* usage in mobile scripts
echo
echo "=== console.* usage in apps/mobile/scripts ==="
rg -n --no-heading "console\.(log|error|warn|info|debug|trace)" apps/mobile/scripts || true
# Search for any logging utility/pattern used by scripts (common candidates)
echo
echo "=== Search for possible logging utilities in repo ==="
rg -n --no-heading "from ['\"](.*logger|.*log|loggers?)[/\"]|createLogger|getLogger|logger\.|pino|winston|bunyan" . -S --glob='*.ts' --glob='*.tsx' --glob='*.js' --glob='*.mjs' --glob='*.cjs' || true
# Inspect build-dev.ts if present
echo
echo "=== apps/mobile/scripts/build-dev.ts (relevant lines) ==="
if [ -f "apps/mobile/scripts/build-dev.ts" ]; then
nl -ba apps/mobile/scripts/build-dev.ts | sed -n '1,160p'
else
echo "MISSING: apps/mobile/scripts/build-dev.ts"
fiRepository: acarlson33/firepit
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
print_with_numbers () {
# usage: print_with_numbers <file> <start> <end>
local file="$1" start="$2" end="$3"
if [ ! -f "$file" ]; then
echo "MISSING: $file"
return 0
fi
awk -v s="$start" -v e="$end" '{ if (NR>=s && NR<=e) printf "%5d\t%s\n", NR, $0 }' "$file"
}
echo "=== Verify file existence ==="
ls -la apps/mobile/scripts 2>/dev/null || true
test -f apps/mobile/scripts/build.ts && echo "FOUND apps/mobile/scripts/build.ts" || echo "NOT FOUND apps/mobile/scripts/build.ts"
test -f apps/mobile/scripts/build-dev.ts && echo "FOUND apps/mobile/scripts/build-dev.ts" || echo "NOT FOUND apps/mobile/scripts/build-dev.ts"
echo
echo "=== apps/mobile/scripts/build.ts (lines 1-80) ==="
print_with_numbers apps/mobile/scripts/build.ts 1 80
echo
echo "=== console.* usage in apps/mobile/scripts ==="
rg -n --no-heading "console\.(log|error|warn|info|debug|trace)" apps/mobile/scripts || true
echo
echo "=== Search for possible logging utilities/patterns in repo (ts/js only) ==="
rg -n --no-heading "console\.(log|error|warn|info|debug|trace)" . -S --glob='*.ts' --glob='*.tsx' --glob='*.js' --glob='*.mjs' --glob='*.cjs' || true
# Try to find common logger imports/usage
rg -n --no-heading "from ['\"](.*logger|.*log|loggers?)[/\"]|createLogger|getLogger|logger\.|pino|winston|bunyan" . -S --glob='*.ts' --glob='*.tsx' --glob='*.js' --glob='*.mjs' --glob='*.cjs' || true
echo
echo "=== apps/mobile/scripts/build-dev.ts (lines 1-120) ==="
print_with_numbers apps/mobile/scripts/build-dev.ts 1 120Repository: acarlson33/firepit
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== process.stdout.write / process.stderr.write usage in scripts ==="
rg -n --no-heading "process\.(stdout|stderr)\.write" . -S --glob='*.ts' --glob='*.tsx' --glob='*.js' --glob='*.mjs' --glob='*.cjs' --glob='*.mts' --glob='*.cts' --glob='*.jsx' --glob='*.mts' --glob='*.cts' || true
echo
echo "=== logger imports/usages in scripts folders ==="
rg -n --no-heading "from ['\"][^'\"]*logger[^'\"]*['\"]|import\s+{[^}]*logger[^}]*}\s+from|client-logger|newrelic-utils" apps --glob='*.ts' --glob='*.tsx' || true
echo
echo "=== Any explicit suppression for no-console in scripts? ==="
rg -n --no-heading "noConsole|biome-ignore.*noConsole|eslint-disable.*no-console" apps --glob='*.ts' --glob='*.tsx' --glob='*.js' || true
echo
echo "=== Show full content of apps/mobile/scripts/build.ts and build-dev.ts (small files) ==="
wc -l apps/mobile/scripts/build.ts apps/mobile/scripts/build-dev.ts
sed -n '1,160p' apps/mobile/scripts/build.ts
echo "----"
sed -n '1,200p' apps/mobile/scripts/build-dev.tsRepository: acarlson33/firepit
Length of output: 5040
Remove console.* logging from the mobile build scripts (use process.stdout.write / process.stderr.write).
apps/mobile/scripts/build.ts(lines 9, 17, 22, 24): replaceconsole.log/console.errorwithprocess.stdout.writefor normal messages andprocess.stderr.writefor failures (per guideline: “Don’t use console”).apps/mobile/scripts/build-dev.ts(lines 9, 23, 31, 33): same replacement.
🤖 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 `@apps/mobile/scripts/build.ts` around lines 9 - 10, Replace all console.*
calls in the mobile build scripts with process.stdout.write for normal messages
and process.stderr.write for errors: in build.ts and build-dev.ts find
occurrences of console.log and console.error (the messages surrounding the
execSync("bunx eas-cli build --platform ios --local", ...) call and similar
execSync calls) and change them to process.stdout.write("...\\n") for
informational output and process.stderr.write("...\\n") for failures; ensure
newline characters are preserved and keep existing message text and surrounding
logic unchanged.
| null, | ||
| [channels, selectedChannelId], | ||
| ); | ||
| const onlineCount = selectedServer?.memberCount ?? channels.length; |
Contributor
There was a problem hiding this comment.
Avoid using channel count as an “online” fallback.
The fallback currently displays channels.length as online users, which is a misleading user-facing metric.
Suggested fix
- const onlineCount = selectedServer?.memberCount ?? channels.length;
+ const memberCount = selectedServer?.memberCount;
...
- {onlineCount} online
+ {memberCount != null
+ ? `${memberCount} members`
+ : "Member count unavailable"}Also applies to: 597-603
🤖 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 `@apps/mobile/src/app/`(tabs)/chat.tsx at line 91, The onlineCount fallback is
incorrectly using channels.length, which misrepresents online users; change the
logic in the onlineCount computation (and the similar occurrences around lines
597-603) to not use channels.length as the fallback—use
selectedServer?.memberCount ?? 0 (or undefined if callers handle missing data)
or a proper online/member presence metric instead, and update any downstream
uses that expect a numeric count accordingly (refer to the onlineCount variable,
selectedServer.memberCount, and channels usage to locate the spots to fix).
Comment on lines
+64
to
+77
| const { instanceUrl, state, authenticate, currentUser, resetConnection } = | ||
| useFirepitBootstrap(); | ||
| const theme = useTheme(); | ||
| const [email, setEmail] = useState(""); | ||
| const [password, setPassword] = useState(""); | ||
| const [authError, setAuthError] = useState<string | null>(null); | ||
|
|
||
| const signedIn = state === "ready" && Boolean(currentUser); | ||
| const signedIn = state === "ready" && Boolean(currentUser); | ||
|
|
||
| useEffect(() => { | ||
| if (signedIn) { | ||
| router.replace("/explore"); | ||
| } | ||
| }, [signedIn]); | ||
| useEffect(() => { | ||
| if (signedIn) { | ||
| router.replace("/home"); | ||
| } | ||
| }, [signedIn]); |
Contributor
There was a problem hiding this comment.
Use the same authenticated predicate as AuthRouteGuard.
This screen treats state === "ready" && currentUser as signed-in, but guarded tab routes require accessToken too. Keep these checks consistent to avoid redirect ping-pong.
Suggested fix
- const { instanceUrl, state, authenticate, currentUser, resetConnection } =
+ const {
+ instanceUrl,
+ state,
+ authenticate,
+ currentUser,
+ accessToken,
+ resetConnection,
+ } =
useFirepitBootstrap();
- const signedIn = state === "ready" && Boolean(currentUser);
+ const signedIn =
+ state === "ready" && Boolean(accessToken) && Boolean(currentUser);🤖 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 `@apps/mobile/src/app/login.tsx` around lines 64 - 77, The signed-in predicate
here is inconsistent with AuthRouteGuard: include accessToken in the bootstrap
destructure (add accessToken from useFirepitBootstrap) and change signedIn to
the same check used by AuthRouteGuard (e.g., state === "ready" &&
Boolean(currentUser) && Boolean(accessToken)) so redirects match guarded routes.
Comment on lines
+17
to
22
| setJWT() { | ||
| return this; | ||
| } | ||
| setSession() { | ||
| return this; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
The new bearer-token tests don't verify the branch this PR adds.
MockClient.setJWT() and .setSession() are both no-ops, so these tests still pass if getSessionFromHeader() picks the wrong auth mode every time. Please record which setter was invoked and assert "session" for opaque bearer tokens and "jwt" for JWT-shaped ones.
Also applies to: 143-173
🤖 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 `@apps/web/src/__tests__/auth-server.test.ts` around lines 17 - 22, The tests
currently use no-op MockClient.setJWT() and setSession(), so they don't verify
the new branch in getSessionFromHeader(); change the mock implementation to
record which setter was called (e.g. set a property like lastAuthMode = "jwt" or
"session" inside MockClient.setJWT and MockClient.setSession) and update the
tests that exercise opaque bearer vs JWT-shaped tokens (tests around lines 17-22
and 143-173) to assert that mockClient.lastAuthMode === "session" for opaque
tokens and === "jwt" for JWT-shaped tokens; keep getSessionFromHeader behavior
unchanged, only add the recording and assertions in the tests to validate the
correct branch is used.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/web/src/lib/auth-server.ts`:
- Around line 99-100: The code performs a redundant case-insensitive lookup for
the Authorization header by calling headerStore.get("Authorization") ??
headerStore.get("authorization"); replace this with a single get call (e.g.,
headerStore.get("authorization") or headerStore.get("Authorization")) since the
Fetch/Next.js Headers implementation used by headerStore already handles
case-insensitive header names; update the assignment to const authHeader =
headerStore.get("authorization") and remove the second conditional branch and
any related comment.
🪄 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: e9faf390-50ac-4d74-b238-6f98cacf9799
📒 Files selected for processing (1)
apps/web/src/lib/auth-server.ts
Comment on lines
+99
to
+100
| // Header name may be in any case; try both | ||
| const authHeader = headerStore.get("Authorization") ?? headerStore.get("authorization"); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Redundant case-insensitive header lookup.
HTTP headers are case-insensitive per RFC 7230, and Headers.get() from the Fetch API (which Next.js's headers() conforms to) already performs case-insensitive matching. Checking both "Authorization" and "authorization" is unnecessary.
♻️ Suggested simplification
- // Header name may be in any case; try both
- const authHeader = headerStore.get("Authorization") ?? headerStore.get("authorization");
+ const authHeader = headerStore.get("Authorization");🤖 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 `@apps/web/src/lib/auth-server.ts` around lines 99 - 100, The code performs a
redundant case-insensitive lookup for the Authorization header by calling
headerStore.get("Authorization") ?? headerStore.get("authorization"); replace
this with a single get call (e.g., headerStore.get("authorization") or
headerStore.get("Authorization")) since the Fetch/Next.js Headers implementation
used by headerStore already handles case-insensitive header names; update the
assignment to const authHeader = headerStore.get("authorization") and remove the
second conditional branch and any related comment.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/src/lib/auth-server.ts (1)
118-136:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStop selecting Appwrite auth mode by token shape.
chosenstill depends onisLikelyJwt(token), but Appwrite documents JWTs as a distinct auth mechanism while session credentials are exposed as opaquesecretstrings without a documented character-set guarantee. That makesheader.payload.signatureshape checks an unsafe discriminator here; a session secret containing.would be routed throughsetJWT()instead ofsetSession(). Prefer an explicit token type or a deterministic fallback strategy instead of dot-count heuristics. (appwrite.io)🤖 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 `@apps/web/src/lib/auth-server.ts` around lines 118 - 136, The code currently picks auth mode using isLikelyJwt(token) (setting chosen) which misroutes opaque Appwrite session secrets containing dots; replace this heuristic with an explicit token type or deterministic fallback: remove isLikelyJwt-based branching and require/parse an explicit auth-type (e.g. incoming header like X-Auth-Type or a tokenType argument) and set chosen from that, or if none provided default to "session"; update the call site getSessionForToken(endpoint, project, token, systemSenderUserId, chosen) to use the explicit/defaulted chosen value and ensure downstream uses (setJWT / setSession) follow the same explicit tokenType contract.
🤖 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 `@apps/web/src/lib/auth-server.ts`:
- Around line 120-143: Remove the console.log debug prints guarding on
FIREPIT_DEBUG_AUTH in auth-server.ts (the blocks that mask the header token and
that print the session result around the getSessionForToken call); instead
either drop them or route the same info through the project’s redacted server
logger (e.g., use the existing redacted/server logger instance) and ensure you
never emit raw tokens or unredacted userId—keep the masked token and redact or
omit session.$id when logging; update both debug blocks (the
token-masking/logging before calling getSessionForToken and the session-result
logging after) to use that logger or be removed entirely.
---
Duplicate comments:
In `@apps/web/src/lib/auth-server.ts`:
- Around line 118-136: The code currently picks auth mode using
isLikelyJwt(token) (setting chosen) which misroutes opaque Appwrite session
secrets containing dots; replace this heuristic with an explicit token type or
deterministic fallback: remove isLikelyJwt-based branching and require/parse an
explicit auth-type (e.g. incoming header like X-Auth-Type or a tokenType
argument) and set chosen from that, or if none provided default to "session";
update the call site getSessionForToken(endpoint, project, token,
systemSenderUserId, chosen) to use the explicit/defaulted chosen value and
ensure downstream uses (setJWT / setSession) follow the same explicit tokenType
contract.
🪄 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: 470b6d71-14bd-4c43-9ba6-de231c966657
📒 Files selected for processing (1)
apps/web/src/lib/auth-server.ts
Comment on lines
+120
to
+143
| if (process.env.FIREPIT_DEBUG_AUTH === "true") { | ||
| const masked = | ||
| token.length > 8 | ||
| ? `${token.slice(0, 4)}...${token.slice(-4)}` | ||
| : token; | ||
| // eslint-disable-next-line no-console | ||
| console.log( | ||
| `[auth-debug] header token present, chosen=${chosen}, token="${masked}", endpoint=${endpoint}, project=${project}`, | ||
| ); | ||
| } | ||
|
|
||
| const session = await getSessionForToken( | ||
| endpoint, | ||
| project, | ||
| token, | ||
| systemSenderUserId, | ||
| "jwt", | ||
| chosen, | ||
| ); | ||
|
|
||
| if (process.env.FIREPIT_DEBUG_AUTH === "true") { | ||
| // eslint-disable-next-line no-console | ||
| console.log( | ||
| `[auth-debug] header auth result: ${session ? `userId=${session.$id}` : "no session"}`, | ||
| ); |
Contributor
There was a problem hiding this comment.
Remove console-based auth debug logging from this path.
These new console.log calls emit authentication context (userId, endpoint/project, masked token) from request handling. Even behind FIREPIT_DEBUG_AUTH, that is a bad fit for a sensitive auth path and also violates the repository rule against console. Route this through the existing redacted server logger, or drop the logs entirely.
As per coding guidelines, "Don't use console".
Also applies to: 165-188
🤖 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 `@apps/web/src/lib/auth-server.ts` around lines 120 - 143, Remove the
console.log debug prints guarding on FIREPIT_DEBUG_AUTH in auth-server.ts (the
blocks that mask the header token and that print the session result around the
getSessionForToken call); instead either drop them or route the same info
through the project’s redacted server logger (e.g., use the existing
redacted/server logger instance) and ensure you never emit raw tokens or
unredacted userId—keep the masked token and redact or omit session.$id when
logging; update both debug blocks (the token-masking/logging before calling
getSessionForToken and the session-result logging after) to use that logger or
be removed entirely.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/web/src/__tests__/newrelic-utils.test.ts`:
- Around line 88-93: The tests use expect.any(Number) for severity in
mockEmitPostHogLog assertions which won't catch incorrect level-to-severity
mappings; update each assertion in newrelic-utils.test.ts that checks
mockEmitPostHogLog to assert the exact SeverityNumber enum value instead of
expect.any(Number) (e.g., assert SeverityNumber.INFO for info-level messages and
SeverityNumber.ERROR for recordError), and apply the same change to the other
failing blocks referenced (lines covering other level checks) so each
expect.objectContaining({ severityNumber: ... }) compares to the specific
SeverityNumber constant.
In `@apps/web/src/lib/newrelic-utils.ts`:
- Around line 351-362: The OTLP/PostHog emission in recordError() is missing the
provider gate; update recordError() to call shouldSendToPostHog() before
invoking emitPostHogLog() and schedulePostHogLogFlush() (same pattern used where
emitPostHogLog({...}) is already gated), so that emitPostHogLog and
schedulePostHogLogFlush are only executed when shouldSendToPostHog() returns
true; locate the recordError function and wrap the PostHog-specific emission
calls with that conditional, preserving the existing payload structure
(errorObject.message, SeverityNumber.ERROR, attributes including
errorMessage/errorName/errorStack and ...customAttributes).
- Around line 286-304: The PostHog emission code currently calls
emitPostHogLog(...) and schedulePostHogLogFlush() unconditionally; wrap this
entire emission path in a guard that calls shouldSendToPostHog() and only
proceeds when it returns true. Locate the block that calls emitPostHogLog and
schedulePostHogLogFlush and add a conditional check using shouldSendToPostHog();
if false, skip both calls entirely so OTLP logs are not emitted when the
telemetry provider is set to newrelic/none. Ensure the existing payload
(body/message, severityNumber, attributes) remains unchanged inside the guarded
block.
🪄 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: 6be7dd5a-54d5-473a-9cfe-eb7c47ff215b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lockand included bybun.lock
📒 Files selected for processing (6)
apps/mobile/package.jsonapps/web/instrumentation.tsapps/web/package.jsonapps/web/src/__tests__/newrelic-utils.test.tsapps/web/src/lib/newrelic-utils.tsapps/web/src/lib/posthog-logs.ts
Comment on lines
+88
to
+93
| expect(mockEmitPostHogLog).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| body: "Test info message", | ||
| severityNumber: expect.any(Number), | ||
| }), | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Assert exact severity mapping, not just “any number.”
Using expect.any(Number) won’t catch broken level-to-severity mapping. Please assert exact SeverityNumber values per level (and SeverityNumber.ERROR for recordError).
Suggested test tightening
+import { SeverityNumber } from "`@opentelemetry/api-logs`";
...
- severityNumber: expect.any(Number),
+ severityNumber: SeverityNumber.INFO,
...
- severityNumber: expect.any(Number),
+ severityNumber: SeverityNumber.ERROR,
...
- severityNumber: expect.any(Number),
+ severityNumber: SeverityNumber.WARN,
...
- severityNumber: expect.any(Number),
+ severityNumber: SeverityNumber.DEBUG,
...
- severityNumber: expect.any(Number),
+ severityNumber: SeverityNumber.ERROR,Also applies to: 99-105, 111-116, 122-128, 134-139, 145-151, 157-162, 168-174, 183-192, 208-217
🤖 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 `@apps/web/src/__tests__/newrelic-utils.test.ts` around lines 88 - 93, The
tests use expect.any(Number) for severity in mockEmitPostHogLog assertions which
won't catch incorrect level-to-severity mappings; update each assertion in
newrelic-utils.test.ts that checks mockEmitPostHogLog to assert the exact
SeverityNumber enum value instead of expect.any(Number) (e.g., assert
SeverityNumber.INFO for info-level messages and SeverityNumber.ERROR for
recordError), and apply the same change to the other failing blocks referenced
(lines covering other level checks) so each expect.objectContaining({
severityNumber: ... }) compares to the specific SeverityNumber constant.
Comment on lines
+286
to
+304
| emitPostHogLog({ | ||
| body: message, | ||
| severityNumber: | ||
| level === LogLevel.ERROR | ||
| ? SeverityNumber.ERROR | ||
| : level === LogLevel.WARN | ||
| ? SeverityNumber.WARN | ||
| : level === LogLevel.DEBUG | ||
| ? SeverityNumber.DEBUG | ||
| : SeverityNumber.INFO, | ||
| attributes: { | ||
| level, | ||
| message, | ||
| timestamp: new Date().toISOString(), | ||
| ...attributes, | ||
| }, | ||
| }); | ||
| schedulePostHogLogFlush(); | ||
|
|
Contributor
There was a problem hiding this comment.
Guard PostHog log emission behind shouldSendToPostHog().
This path currently emits OTLP logs even when telemetry provider is configured to newrelic/none, as long as credentials exist. That bypasses the provider gate and can leak unexpected telemetry.
Suggested fix
- emitPostHogLog({
- body: message,
- severityNumber:
- level === LogLevel.ERROR
- ? SeverityNumber.ERROR
- : level === LogLevel.WARN
- ? SeverityNumber.WARN
- : level === LogLevel.DEBUG
- ? SeverityNumber.DEBUG
- : SeverityNumber.INFO,
- attributes: {
- level,
- message,
- timestamp: new Date().toISOString(),
- ...attributes,
- },
- });
- schedulePostHogLogFlush();
+ if (shouldSendToPostHog()) {
+ emitPostHogLog({
+ body: message,
+ severityNumber:
+ level === LogLevel.ERROR
+ ? SeverityNumber.ERROR
+ : level === LogLevel.WARN
+ ? SeverityNumber.WARN
+ : level === LogLevel.DEBUG
+ ? SeverityNumber.DEBUG
+ : SeverityNumber.INFO,
+ attributes: {
+ level,
+ message,
+ timestamp: new Date().toISOString(),
+ ...attributes,
+ },
+ });
+ schedulePostHogLogFlush();
+ }🤖 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 `@apps/web/src/lib/newrelic-utils.ts` around lines 286 - 304, The PostHog
emission code currently calls emitPostHogLog(...) and schedulePostHogLogFlush()
unconditionally; wrap this entire emission path in a guard that calls
shouldSendToPostHog() and only proceeds when it returns true. Locate the block
that calls emitPostHogLog and schedulePostHogLogFlush and add a conditional
check using shouldSendToPostHog(); if false, skip both calls entirely so OTLP
logs are not emitted when the telemetry provider is set to newrelic/none. Ensure
the existing payload (body/message, severityNumber, attributes) remains
unchanged inside the guarded block.
Comment on lines
+351
to
+362
| emitPostHogLog({ | ||
| body: errorObject.message, | ||
| severityNumber: SeverityNumber.ERROR, | ||
| attributes: { | ||
| errorMessage: errorObject.message, | ||
| errorName: errorObject.name, | ||
| errorStack: errorObject.stack, | ||
| ...customAttributes, | ||
| }, | ||
| }); | ||
| schedulePostHogLogFlush(); | ||
|
|
Contributor
There was a problem hiding this comment.
Apply the same provider gate in recordError() OTLP emission.
recordError() currently bypasses shouldSendToPostHog() for log export, so it can send error logs when PostHog forwarding is intended to be off.
Suggested fix
- emitPostHogLog({
- body: errorObject.message,
- severityNumber: SeverityNumber.ERROR,
- attributes: {
- errorMessage: errorObject.message,
- errorName: errorObject.name,
- errorStack: errorObject.stack,
- ...customAttributes,
- },
- });
- schedulePostHogLogFlush();
+ if (shouldSendToPostHog()) {
+ emitPostHogLog({
+ body: errorObject.message,
+ severityNumber: SeverityNumber.ERROR,
+ attributes: {
+ errorMessage: errorObject.message,
+ errorName: errorObject.name,
+ errorStack: errorObject.stack,
+ ...customAttributes,
+ },
+ });
+ schedulePostHogLogFlush();
+ }🤖 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 `@apps/web/src/lib/newrelic-utils.ts` around lines 351 - 362, The OTLP/PostHog
emission in recordError() is missing the provider gate; update recordError() to
call shouldSendToPostHog() before invoking emitPostHogLog() and
schedulePostHogLogFlush() (same pattern used where emitPostHogLog({...}) is
already gated), so that emitPostHogLog and schedulePostHogLogFlush are only
executed when shouldSendToPostHog() returns true; locate the recordError
function and wrap the PostHog-specific emission calls with that conditional,
preserving the existing payload structure (errorObject.message,
SeverityNumber.ERROR, attributes including errorMessage/errorName/errorStack and
...customAttributes).
acarlson33
changed the title
feat: appwrite presence API migration for typing statuses chore(deps): update dependencies in repo (both web, root, and mobile) chore(version): increment pacakge/app version (web: 2.0.0-canary.3, mobile: 2.0.0-canary.1)
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 primarily refactors and improves the
SessionScreenand related UI components inapps/mobile/src/app/explore.tsxfor better code readability, maintainability, and user experience. The changes include extracting logic for filtering discoverable servers, enhancing conditional rendering, improving layout and formatting, and making minor UI adjustments. Additionally, there is a small import update inapps/mobile/src/app/server/[serverId].tsx.Logic and Data Handling Improvements:
joinedServerIdsset and adiscoverablePublicServerslist usinguseMemoto ensure that only public servers the user has not joined are shown in the "Public discovery" section. [1] [2]discoverablePublicServers, and display a fallback message if no public servers are available. [1] [2]UI and Layout Enhancements:
ActionButton,ThemedText,ThemedView, and related components. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14]Conditional Rendering Improvements:
Minor UI/UX Adjustments:
Codebase Maintenance:
apps/mobile/src/app/server/[serverId].tsxto include new data-fetching utilities for channels and servers. (apps/mobile/src/app/server/[serverId].tsxL23-R28)