Add markdown rendering for chat messages#6
Conversation
|
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:
📝 WalkthroughWalkthroughChatMessage now renders user messages as selectable themed text; assistant messages are rendered inside a themed view and passed to a new MarkdownRenderer that normalizes and renders Markdown with theme-aware styles and link handling. Multiple frontend dependency, config, import, and hook-memoization adjustments were applied. Changes
Sequence DiagramsequenceDiagram
participant User
participant ChatMessage
participant MarkdownRenderer
participant MD_Display as react-native-markdown-display
participant Linking
User->>ChatMessage: deliver message object
alt message.role == "user"
ChatMessage->>ChatMessage: render ThemedText (plain, selectable)
else
ChatMessage->>MarkdownRenderer: pass message.text
MarkdownRenderer->>MarkdownRenderer: normalize text & compute theme styles (useColorScheme)
MarkdownRenderer->>MD_Display: render markdown with theme-aware styles
MD_Display->>Linking: onLinkPress -> openURL(url) (errors suppressed)
MD_Display-->>MarkdownRenderer: rendered elements
MarkdownRenderer-->>User: display formatted content
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@front/components/ChatMessage.tsx`:
- Line 9: The import for MarkdownRenderer in ChatMessage.tsx uses a default
import but the component is exported as a named export; change the import
statement to a named import (import { MarkdownRenderer } from
"@/components/MarkdownRenderer") so it matches the component's named export and
coding guidelines, updating any references in this file that assume the default
import to use the named symbol MarkdownRenderer.
In `@front/components/MarkdownRenderer.tsx`:
- Line 136: The file currently uses a default export for the React component;
change it to a named export by exporting the MarkdownRenderer symbol as a named
export (e.g., export const MarkdownRenderer = ...) or by replacing the final
default export with a named export (export { MarkdownRenderer }); then update
all import sites to use the named import (import { MarkdownRenderer } from
'...') and ensure the component file stays a .tsx TypeScript component.
- Line 39: Update the KaTeX CDN stylesheet URL in MarkdownRenderer.tsx so its
version matches the installed runtime (package.json's ^0.16.45); locate the
<link rel="stylesheet"
href="https://cdn.jsdelivr.net/npm/katex@0.16.9/dist/katex.min.css"> in the
MarkdownRenderer component and change the version segment to 0.16.45 (or the
exact runtime version) so the CSS and KaTeX runtime are identical.
- Around line 33-34: The parsed HTML from marked.parse (htmlContent) is injected
into the WebView (component using originWhitelist) without sanitization creating
XSS risk; fix by sanitizing the parsed HTML output with a library like
sanitize-html or isomorphic-dompurify before assigning to htmlContent and use
that sanitized string for the WebView source, and harden the WebView props by
tightening originWhitelist (avoid ['*']), disabling/excluding JavaScript
execution (e.g., javaScriptEnabled=false), disabling DOM storage, and removing
permissive settings so only trusted origins are allowed; update references to
marked.parse, htmlContent, and the WebView component props accordingly.
- Around line 125-132: Extract the inline style object from the WebView into a
StyleSheet by creating e.g. const styles = StyleSheet.create({ webview: {
backgroundColor: 'transparent', minHeight: 50 } }) and apply it to the WebView
via style={styles.webview}; additionally add startInLoadingState={true} and a
renderError prop that returns a simple fallback component (e.g. a View/Text) to
the WebView element so the MarkdownRenderer component handles loading and error
states properly.
- Around line 18-24: Update the inline LaTeX regex in MarkdownRenderer.tsx that
currently uses the capturing group ([^)]+) which fails on expressions with
nested parentheses; change that group to a non-greedy [\s\S]+? (matching any
char including newlines) so the .replace call that renders inline math via
katex.renderToString(latex, { throwOnError: false, displayMode: false }) will
capture full expressions like \(2(3*6)+(82-(a+5))=a+4\); keep the existing
try/catch and do not alter the separate display-mode pattern.
In `@front/package.json`:
- Around line 49-50: Jest will fail to parse the new ESM-only dependency
"marked" (and potentially the ESM build of "katex"), so update your Jest config
(jest.config.js or the "jest" field) to ensure node_modules/marked and
node_modules/katex are transformed (i.e., modify transformIgnorePatterns to
exclude these packages from being ignored, for example by using a negative
lookahead like /(?!marked|katex)/ in the pattern); target the
transformIgnorePatterns setting and add "marked" and "katex" so Babel/jest can
transpile them during tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5f5ce6b-eba6-4f28-aa8f-a1a841653c63
⛔ Files ignored due to path filters (1)
front/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
front/components/ChatMessage.tsxfront/components/MarkdownRenderer.tsxfront/package.json
| import { ActivityIndicator } from "react-native"; | ||
| import { useThemeColor } from "@/hooks/useThemeColor"; | ||
| import { IconSymbol } from "@/components/ui/IconSymbol"; | ||
| import MarkdownRenderer from "@/components/MarkdownRenderer"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use named import for MarkdownRenderer.
Line 9 should switch to a named import to match the component export convention.
Suggested change
-import MarkdownRenderer from "@/components/MarkdownRenderer";
+import { MarkdownRenderer } from "@/components/MarkdownRenderer";As per coding guidelines, "Place React Native components in the components directory, use TypeScript for all component files, and export components as named exports".
📝 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.
| import MarkdownRenderer from "@/components/MarkdownRenderer"; | |
| import { MarkdownRenderer } from "@/components/MarkdownRenderer"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/components/ChatMessage.tsx` at line 9, The import for MarkdownRenderer
in ChatMessage.tsx uses a default import but the component is exported as a
named export; change the import statement to a named import (import {
MarkdownRenderer } from "@/components/MarkdownRenderer") so it matches the
component's named export and coding guidelines, updating any references in this
file that assume the default import to use the named symbol MarkdownRenderer.
| <WebView | ||
| originWhitelist={['*']} | ||
| source={{ html }} | ||
| style={{ backgroundColor: 'transparent', minHeight: 50 }} | ||
| scrollEnabled={false} | ||
| showsVerticalScrollIndicator={false} | ||
| messagingEnabled={false} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n front/components/MarkdownRenderer.tsx | head -150Repository: tpaulshippy/bots
Length of output: 4363
Move inline style to StyleSheet.create and add WebView loading/error handling.
Line 128 uses an inline style object that should be extracted to StyleSheet.create. Additionally, add startInLoadingState and renderError props to provide proper loading and error states per the coding guidelines for this component directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/components/MarkdownRenderer.tsx` around lines 125 - 132, Extract the
inline style object from the WebView into a StyleSheet by creating e.g. const
styles = StyleSheet.create({ webview: { backgroundColor: 'transparent',
minHeight: 50 } }) and apply it to the WebView via style={styles.webview};
additionally add startInLoadingState={true} and a renderError prop that returns
a simple fallback component (e.g. a View/Text) to the WebView element so the
MarkdownRenderer component handles loading and error states properly.
| ); | ||
| }; | ||
|
|
||
| export default MarkdownRenderer; No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Export this component as a named export.
Switch from default export to named export, then update import sites accordingly.
Suggested change
-const MarkdownRenderer = ({ content }: MarkdownRendererProps) => {
+export const MarkdownRenderer = ({ content }: MarkdownRendererProps) => {
// ...
};
-
-export default MarkdownRenderer;As per coding guidelines, "Place React Native components in the components directory, use TypeScript for all component files, and export components as named exports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/components/MarkdownRenderer.tsx` at line 136, The file currently uses a
default export for the React component; change it to a named export by exporting
the MarkdownRenderer symbol as a named export (e.g., export const
MarkdownRenderer = ...) or by replacing the final default export with a named
export (export { MarkdownRenderer }); then update all import sites to use the
named import (import { MarkdownRenderer } from '...') and ensure the component
file stays a .tsx TypeScript component.
- Upgraded @react-native-async-storage/async-storage to 2.2.0 - Upgraded @react-native-picker/picker to 2.11.4 - Upgraded @sentry/react-native to ~7.11.0 - Updated Expo packages to version 55.x - Upgraded React and React DOM to 19.2.0 - Upgraded React Native to 0.83.4 - Updated react-native-gesture-handler to ~2.30.0 - Updated react-native-reanimated to 4.2.1 - Updated react-native-safe-area-context to ~5.6.2 - Updated react-native-screens to ~4.23.0 - Updated react-native-svg to 15.15.3 - Updated react-native-webview to 13.16.0 - Added react-native-worklets to 0.7.2 - Updated @types/react to ~19.2.10 - Updated eslint-config-expo to ~55.0.0 - Updated jest-expo to ~55.0.15 - Updated typescript to ~5.9.2
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@front/components/MarkdownRenderer.tsx`:
- Around line 136-145: The Markdown output currently leaves inline LaTeX like
\(a=54.5\) as plain text; add a custom rule to the Markdown component in
MarkdownRenderer.tsx that detects inline math delimiters (e.g. /\\(.*?\)/) and
renders them with KaTeX instead of plain text: create a rules prop (e.g.
math_inline) for the <Markdown> component that parses matched content, strips
the delimiters, passes the inner TeX to a KaTeX renderer (or react-native-katex
component / renderToString from 'katex') and returns a React element; ensure you
pass the new rules prop to the existing <Markdown style={markdownStyles} ...>
and keep normalizedContent as the source, falling back to plain text on KaTeX
render errors.
- Around line 139-141: The onLinkPress handler currently opens any URL via
Linking.openURL which allows unsafe schemes; update the handler in
MarkdownRenderer.tsx to parse the incoming url (use the URL constructor or
equivalent) and only call Linking.openURL for allowlisted schemes (at minimum
"http" and "https"); for non-allowlisted schemes simply return true (or no-op)
and do not call Linking.openURL. Reference the onLinkPress function and the
Linking.openURL call when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec51de90-680c-417f-a8ee-96bf5e18a64d
⛔ Files ignored due to path filters (1)
front/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
front/app.jsonfront/components/MarkdownRenderer.tsxfront/package.json
| return ( | ||
| <Markdown | ||
| style={markdownStyles} | ||
| onLinkPress={(url: string) => { | ||
| Linking.openURL(url).catch(() => null); | ||
| return true; | ||
| }} | ||
| > | ||
| {normalizedContent} | ||
| </Markdown> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does react-native-markdown-display 7.0.2 support LaTeX delimiters like ( ... ) natively, or is a custom rule/plugin required to render KaTeX?
💡 Result:
No, react-native-markdown-display 7.0.2 does not support LaTeX delimiters like ( ... ) natively. A custom rule or plugin is required to render KaTeX.
Citations:
- 1: https://stackoverflow.com/questions/79353967/is-there-any-way-to-show-latex-in-markdown-react-native
- 2: https://www.npmjs.com/package/react-native-markdown-display
- 3: https://github.com/iamacup/react-native-markdown-display
- 4: Rendering math Equation iamacup/react-native-markdown-display#211
- 5: https://github.com/iamacup/react-native-markdown-display/releases
Add custom KaTeX rendering for math delimiters in Markdown.
The react-native-markdown-display library does not support LaTeX delimiters like \( ... \) natively. Custom rule implementation using KaTeX is required to render math expressions. Currently, inputs like \(a=54.5\) will remain as plain text, leaving the PR's LaTeX rendering objective unmet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/components/MarkdownRenderer.tsx` around lines 136 - 145, The Markdown
output currently leaves inline LaTeX like \(a=54.5\) as plain text; add a custom
rule to the Markdown component in MarkdownRenderer.tsx that detects inline math
delimiters (e.g. /\\(.*?\)/) and renders them with KaTeX instead of plain text:
create a rules prop (e.g. math_inline) for the <Markdown> component that parses
matched content, strips the delimiters, passes the inner TeX to a KaTeX renderer
(or react-native-katex component / renderToString from 'katex') and returns a
React element; ensure you pass the new rules prop to the existing <Markdown
style={markdownStyles} ...> and keep normalizedContent as the source, falling
back to plain text on KaTeX render errors.
| onLinkPress={(url: string) => { | ||
| Linking.openURL(url).catch(() => null); | ||
| return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In React Native 0.83 / Expo 55, what is the recommended pattern for safely handling untrusted markdown links with Linking.openURL—should apps allowlist schemes like https/http instead of passing arbitrary URLs through?
💡 Result:
Yes, apps should allowlist schemes like https/http instead of passing arbitrary URLs through to Linking.openURL when handling untrusted markdown links in React Native 0.83 / Expo 55. The recommended pattern is to use a markdown renderer that provides an onLinkPress (or equivalent) handler, such as react-native-markdown-display or react-native-render-html. In the handler, validate the URL before calling Linking.openURL: 1. Parse/validate it's a proper URL (e.g., using URL constructor or regex). 2. Check the scheme is http/https (and optionally tel/mailto/sms if needed). 3. Optionally check canOpenURL. 4. If valid, call Linking.openURL(url); else ignore or show error. Official RN/Expo docs note to use canOpenURL before non-http(s) schemes but do not explicitly warn against arbitrary URLs. However, security best practices for untrusted content require allowlisting to prevent malicious schemes (e.g., custom app schemes, javascript:, file:) from executing harmful actions like opening other apps, dialing numbers, or leaking data. Example with react-native-markdown-display (common for markdown): const onLinkPress = (url) => { try { const parsed = new URL(url); if (parsed.protocol === 'https:' || parsed.protocol === 'http:') { Linking.openURL(url); return true; } } catch {} return false; // block }; ... react-native-render-html uses renderersProps.a.onPress for similar validation. This prevents open redirect/phishing attacks via malicious markdown.
Citations:
- 1: https://reactnative.dev/docs/0.83/linking
- 2: https://docs.expo.dev/linking/into-other-apps
- 3: https://reactnative.dev/docs/linking
- 4: https://docs.expo.dev/versions/v55.0.0/sdk/linking
- 5: https://stackoverflow.com/questions/64699801/linking-canopenurl-returning-false-when-targeting-android-30-sdk-on-react-native
- 6: https://stackoverflow.com/questions/41440825/react-native-javascript-validate-url
- 7: How to handle errors where URL is invalid proyecto26/react-native-inappbrowser#257
- 8: https://stackoverflow.com/questions/70078528/what-is-the-regex-for-url-validation-in-react-native
- 9: https://www.npmjs.com/package/react-native-markdown-display
- 10: https://github.com/flowchase/react-native-markdown-display
- 11: https://meliorence.github.io/react-native-render-html/docs/content/anchors
- 12: https://github.com/archriss/react-native-render-html
🏁 Script executed:
# First, locate the MarkdownRenderer.tsx file and check its contents around lines 139-141
find . -name "MarkdownRenderer.tsx" -type f 2>/dev/nullRepository: tpaulshippy/bots
Length of output: 99
🏁 Script executed:
# Read the MarkdownRenderer.tsx file to verify the actual code at lines 139-141
cat -n ./front/components/MarkdownRenderer.tsx | sed -n '130,150p'Repository: tpaulshippy/bots
Length of output: 697
Allowlist safe URL schemes in markdown link handler to prevent arbitrary deep links.
The onLinkPress handler currently accepts any URL without validation, allowing rendered markdown to trigger unsafe schemes (custom app schemes, javascript:, file:, etc.). Restrict to safe schemes like https and http to prevent malicious deep links from executing unintended app navigation.
Suggested fix
onLinkPress={(url: string) => {
- Linking.openURL(url).catch(() => null);
- return true;
+ if (!/^https?:\/\//i.test(url)) {
+ return false;
+ }
+ void Linking.openURL(url);
+ return true;
}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/components/MarkdownRenderer.tsx` around lines 139 - 141, The
onLinkPress handler currently opens any URL via Linking.openURL which allows
unsafe schemes; update the handler in MarkdownRenderer.tsx to parse the incoming
url (use the URL constructor or equivalent) and only call Linking.openURL for
allowlisted schemes (at minimum "http" and "https"); for non-allowlisted schemes
simply return true (or no-op) and do not call Linking.openURL. Reference the
onLinkPress function and the Linking.openURL call when making this change.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
front/app/chatList.tsx (1)
76-114:⚠️ Potential issue | 🟠 MajorEnsure refresh state always clears and pagination closes on empty pages.
refreshingcan get stuck ontrueiffetchChatsthrows (Line 106 path), andhasMoreis left stale whenresultsis empty (Lines 81-84), which can trigger repeated load-more calls.Proposed fix
const refresh = useCallback(async (nextPage: number) => { setRefreshing(true); try { const profileId = await getProfileId(); const data = await fetchChats(profileId, nextPage); - if (!data || data.results.length === 0) { - setRefreshing(false); - return; - } + if (!data || data.results.length === 0) { + setHasMore(false); + return; + } setChats((prevChats) => { if (!data) { return prevChats; } const newChats = groupByDay(data.results); if (nextPage === 1) { return newChats; } else { const mergedChats = { ...prevChats }; Object.entries(newChats).forEach(([day, chats]) => { if (!mergedChats[day]) { mergedChats[day] = []; } mergedChats[day] = [...mergedChats[day], ...chats]; }); return mergedChats; } }); setHasMore(data.next !== null && data.next !== undefined); - setRefreshing(false); } catch (error) { console.log("Caught error in chatList") console.log(error); if (error instanceof UnauthorizedError) { await clearUser(); router.replace("/login"); } + } finally { + setRefreshing(false); } }, [getProfileId, router]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/app/chatList.tsx` around lines 76 - 114, refresh can remain true if fetchChats throws and hasMore isn't updated when results are empty; update the refresh function to always clear refreshing in a finally block and ensure pagination closes on empty pages by calling setHasMore(false) whenever data is falsy or data.results is empty. Specifically, inside refresh (the useCallback), move setRefreshing(false) into a finally so it runs on success or error, and when you detect !data || data.results.length === 0 setHasMore(false) before returning; keep references to getProfileId, fetchChats, groupByDay, setChats, setHasMore and clearUser/router logic unchanged.front/app/parent/notifications.tsx (1)
130-147:⚠️ Potential issue | 🟠 MajorAdd error handling to async effect functions with awaited API calls.
The
handleNotifications()andsetupDevice()functions invoke multiple API calls (e.g.,fetchDeviceByToken(),upsertDevice(),fetchDevice()) without try/catch blocks. When called as fire-and-forget (viavoid), any failures will result in unhandled promise rejections. Wrap the awaited calls in try/catch to handle and report errors explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/app/parent/notifications.tsx` around lines 130 - 147, The async effect handlers handleNotifications and setupDevice perform awaited API calls (fetchDeviceByToken, upsertDevice, fetchDevice, etc.) and are invoked fire-and-forget; wrap the bodies of both async functions in try/catch blocks to catch and handle errors from those awaits, log or report the error (e.g., console.error or the app logger) with context about which operation failed, and ensure the catch path either sets a safe state (e.g., disable notifications) or returns gracefully to avoid unhandled promise rejections.front/app/parent/profileEditor.tsx (1)
50-65:⚠️ Potential issue | 🟠 MajorStop saving when validation fails.
validateProfile()only flipsnameMissing;saveProfile()still callsupsertProfile(profile)for blank names. That means the editor can persist an invalid profile even after marking the field as missing.Suggested fix
- const validateProfile = useCallback(async () => { - setNameMissing(!profile?.name.trim()); - }, [profile?.name]); + const validateProfile = useCallback(() => { + const isValid = Boolean(profile?.name.trim()); + setNameMissing(!isValid); + return isValid; + }, [profile?.name]); const saveProfile = useCallback(async () => { - await validateProfile(); - - if (profile) { - try { - await upsertProfile(profile); - router.back(); - } catch (error) { - Sentry.captureException(error); - } + if (!profile || !validateProfile()) { + return; + } + + try { + await upsertProfile(profile); + router.back(); + } catch (error) { + Sentry.captureException(error); } }, [profile, router, validateProfile]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/app/parent/profileEditor.tsx` around lines 50 - 65, validateProfile currently only updates nameMissing but doesn't communicate validity; modify validateProfile (or make it return a boolean) so it returns whether the profile is valid (e.g., false when profile?.name.trim() is empty) and then update saveProfile to await validateProfile() and early-return when validation fails before calling upsertProfile(profile). Refer to validateProfile, saveProfile, upsertProfile, setNameMissing and profile when making the change.front/app/parent/botAdvanced.tsx (1)
48-64:⚠️ Potential issue | 🟠 MajorValidation doesn't block the save.
validateBot()only schedules error-state updates viasetState. Awaiting it provides no synchronous validation result, sosaveBot()proceeds immediately toupsertBot(bot)regardless of whether the name or model is empty. Invalid bots can be persisted.🛠️ Proposed fix
- const validateBot = useCallback(async () => { - setNameMissing(!bot?.name.trim()); - setModelMissing(!bot?.ai_model); - }, [bot?.ai_model, bot?.name]); + const validateBot = useCallback(() => { + const missingName = !bot.name.trim(); + const missingModel = !bot.ai_model; + + setNameMissing(missingName); + setModelMissing(missingModel); + + return !(missingName || missingModel); + }, [bot.ai_model, bot.name]); - const saveBot = useCallback(async () => { - await validateBot(); + const saveBot = useCallback(async () => { + if (!validateBot()) { + return; + } if (bot) { try { await upsertBot(bot);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/app/parent/botAdvanced.tsx` around lines 48 - 64, validateBot currently only sets state asynchronously so awaiting it doesn't prevent saveBot from persisting invalid bots; change validateBot (used in saveBot) to synchronously compute a boolean validity from bot (e.g., const valid = Boolean(bot && bot.name && bot.name.trim() && bot.ai_model)) then call setNameMissing / setModelMissing with the computed values and return the boolean; update saveBot to call const valid = validateBot(); if (!valid) return early and do not call upsertBot(bot); reference functions validateBot and saveBot and state setters setNameMissing/setModelMissing/upsertBot in your changes.front/app/parent/botSimple.tsx (1)
33-50:⚠️ Potential issue | 🟠 MajorThe editor switch can still save invalid data, and it mutates state in place.
validateBot()only updatesnameMissing, soswitchToAdvancedEditor()still writes the bot even when the name is blank. On top of that,bot.simple_editor = falsemutates the current state object directly.🛠️ Proposed fix
- const validateBot = useCallback(async () => { - setNameMissing(!bot?.name.trim()); - }, [bot?.name]); + const validateBot = useCallback(() => { + const missingName = !bot.name.trim(); + setNameMissing(missingName); + return !missingName; + }, [bot.name]); - const switchToAdvancedEditor = useCallback(async () => { - await validateBot(); + const switchToAdvancedEditor = useCallback(async () => { + if (!validateBot()) { + return; + } + if (bot) { - bot.simple_editor = false; + const nextBot = { ...bot, simple_editor: false }; try { - const newBot = await upsertBot(bot); + const newBot = await upsertBot(nextBot); if (onSwitchEditor && newBot) { onSwitchEditor(newBot); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/app/parent/botSimple.tsx` around lines 33 - 50, validateBot currently only toggles nameMissing but switchToAdvancedEditor still proceeds to save and mutates the bot in-place; change switchToAdvancedEditor to first derive a validation result from bot (e.g., const nameBlank = !bot?.name?.trim()), call setNameMissing(nameBlank) and if nameBlank return early, then create a shallow copy of the bot (do not assign to bot directly) and set copy.simple_editor = false before calling upsertBot(copy); keep existing Sentry error capture and onSwitchEditor(newBot) flow but operate on the copied object to avoid mutating React state.
🧹 Nitpick comments (3)
front/components/__tests__/ThemedText-test.tsx (1)
2-13: Avoid expandingreact-test-rendererusage in React 19 tests.This file is in
__tests__, but the change adds more dependence onreact-test-renderer. React 19 deprecates it, and this repo’s test guidance prefers React Native Testing Library.Suggested refactor
-import renderer, { act } from 'react-test-renderer'; +import { render } from '@testing-library/react-native'; -it(`renders correctly`, async () => { - let tree: renderer.ReactTestRendererJSON | renderer.ReactTestRendererJSON[] | null = null; - - await act(async () => { - tree = renderer.create(<ThemedText>Snapshot test!</ThemedText>).toJSON(); - }); - - expect(tree).toMatchSnapshot(); +it('renders correctly', () => { + const { toJSON } = render(<ThemedText>Snapshot test!</ThemedText>); + expect(toJSON()).toMatchSnapshot(); });As per coding guidelines, "Write tests in tests directories using Jest and React Native Testing Library."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/components/__tests__/ThemedText-test.tsx` around lines 2 - 13, The test currently uses react-test-renderer (imports renderer and act, calls renderer.create(...).toJSON()) which is discouraged for React 19; replace it with React Native Testing Library: remove imports of renderer and act, import { render } from '@testing-library/react-native', call const tree = render(<ThemedText>Snapshot test!</ThemedText>).toJSON() (or const { toJSON } = render(...); expect(toJSON()).toMatchSnapshot()), and keep the assertion on toMatchSnapshot() so the test targets the ThemedText component using render from the testing library.front/eslint.config.js (1)
10-13: Consider adding explicit devDependency for clarity, though transitive installation is currently working.The
typescriptimport resolver is available via the lockfile (installed as a transitive dependency), so a clean install will not break. However,eslint-import-resolver-typescriptis not listed as a direct devDependency infront/package.json. Adding it explicitly would improve clarity and reduce coupling to changes in the transitive dependency chain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/eslint.config.js` around lines 10 - 13, The ESLint import resolver 'typescript' referenced under the 'import/resolver' config is currently satisfied only transitively; add an explicit devDependency "eslint-import-resolver-typescript" to front/package.json (devDependencies) so the resolver is guaranteed present; update package.json accordingly and run npm/yarn install to update lockfile so CI and clean installs won't rely on transitive packages.front/app/parent/botEditor.tsx (1)
20-52: Guard the async load against stale responses.If
botIdchanges or the screen unmounts whilefetchBot()/fetchAiModels()is in flight, a latesetBot()from the older request can overwrite the newer editor state. Add a cancellation guard in the effect, or haveloadSelectedBot()return the loaded bot and only commit it while the effect is still current. Based on learnings, "Use useEffect for side effects and data fetching with proper cleanup".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/app/parent/botEditor.tsx` around lines 20 - 52, loadSelectedBot can commit stale state when fetchBot()/fetchAiModels() resolve after botId changes or unmounts; update the effect to guard commits by either: (A) change loadSelectedBot to return the fetched bot object (or null) instead of calling setBot internally and inside the useEffect call loadSelectedBot().then(fetched => { if (!isStale) setBot(fetched); }) where isStale is a flag set false at start and flipped true in the effect cleanup, or (B) accept an AbortSignal/flag into loadSelectedBot and have it check the signal before calling setBot; ensure the useEffect creates the flag/AbortController, passes it in, and cleans up by marking stale/aborting so late responses never call setBot. Use identifiers loadSelectedBot, fetchBot, fetchAiModels, setBot, useEffect, and local.botId to locate the code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@front/api/account.ts`:
- Around line 17-20: The success checks in the account API calls currently
compare HTTP status codes (e.g., using status === 200 and status === 204) rather
than using the ApiResponse.ok flag used across the codebase; update the handlers
that call apiClient<Account>(...) and apiClient<void>(...) so they validate
response.ok (or !ok) instead of exact status codes, and throw errors when ok is
false while keeping the same error messages and returned data variables (data,
status, ok); reference the apiClient call sites in this file (the functions that
fetch the Account and the one that updates the account) and follow the pattern
used in updateAccount where !ok is already used.
In `@front/app/__tests__/_layout-test.tsx`:
- Around line 180-184: The test fails to exercise the real cleanup because
../_layout is mocked to return null so RootLayout's useEffect never registers
notification listeners; either remove the jest.mock('../_layout', ...) so
render(<RootLayout />) runs the real useEffect (which registers via
Notifications.addNotificationReceivedListener and
Notifications.addNotificationResponseReceivedListener) or, if you must keep the
mock, seed the Notifications.addNotificationReceivedListener and
Notifications.addNotificationResponseReceivedListener mocks with fake
subscription objects that have a remove method before calling render so
(Notifications.addNotificationReceivedListener as
jest.Mock).mock.results[0]?.value and
(Notifications.addNotificationResponseReceivedListener as
jest.Mock).mock.results[0]?.value are defined and their .remove() calls can be
asserted.
In `@front/app/_layout.tsx`:
- Around line 181-196: The effect currently only subscribes to future Linking
"url" events and misses processing a cold-start deep link; update the useEffect
that references getJWTFromLink, initialNavigationChecks, SplashScreen.hideAsync
and loaded to also call Linking.getInitialURL() once during initialization
(before or as part of the initialize async function) and, if it returns a URL,
invoke getJWTFromLink with that URL (handle it the same way the event handler
does), then proceed to addEventListener as before; ensure the initialize
function awaits SplashScreen.hideAsync(), calls initialNavigationChecks(),
safely handles errors from getInitialURL(), and still returns the
subscription.remove() cleanup.
In `@front/app/parent/notifications.tsx`:
- Around line 119-127: The code updates the local deviceRef and calls
updateDeviceState with updatedDevice even if upsertDevice(updatedDevice) fails
or returns null, which can desync client state from the server; change the flow
in the block that constructs updatedDevice (the object with notify_on_new_chat
and notify_on_new_message) to await upsertDevice(updatedDevice), check the
returned value (the upsert result) and only call updateDeviceState and write to
deviceRef.current when the upsert returned a non-null/valid device; if
upsertDevice returns null or throws, preserve the existing deviceRef.current and
handle/log the error instead of applying updatedDevice locally.
In `@front/components/ErrorBoundary.tsx`:
- Around line 26-27: componentDidCatch currently only captures the Error object
and omits React's errorInfo (componentStack); update
ErrorBoundary.componentDidCatch to accept the second parameter (errorInfo) and
include the componentStack when reporting to Sentry—either pass errorInfo (or
errorInfo.componentStack) as Sentry.captureException's second argument or use
Sentry.withScope to add errorInfo.componentStack to the event extras before
calling Sentry.captureException to preserve the React component hierarchy for
debugging.
---
Outside diff comments:
In `@front/app/chatList.tsx`:
- Around line 76-114: refresh can remain true if fetchChats throws and hasMore
isn't updated when results are empty; update the refresh function to always
clear refreshing in a finally block and ensure pagination closes on empty pages
by calling setHasMore(false) whenever data is falsy or data.results is empty.
Specifically, inside refresh (the useCallback), move setRefreshing(false) into a
finally so it runs on success or error, and when you detect !data ||
data.results.length === 0 setHasMore(false) before returning; keep references to
getProfileId, fetchChats, groupByDay, setChats, setHasMore and clearUser/router
logic unchanged.
In `@front/app/parent/botAdvanced.tsx`:
- Around line 48-64: validateBot currently only sets state asynchronously so
awaiting it doesn't prevent saveBot from persisting invalid bots; change
validateBot (used in saveBot) to synchronously compute a boolean validity from
bot (e.g., const valid = Boolean(bot && bot.name && bot.name.trim() &&
bot.ai_model)) then call setNameMissing / setModelMissing with the computed
values and return the boolean; update saveBot to call const valid =
validateBot(); if (!valid) return early and do not call upsertBot(bot);
reference functions validateBot and saveBot and state setters
setNameMissing/setModelMissing/upsertBot in your changes.
In `@front/app/parent/botSimple.tsx`:
- Around line 33-50: validateBot currently only toggles nameMissing but
switchToAdvancedEditor still proceeds to save and mutates the bot in-place;
change switchToAdvancedEditor to first derive a validation result from bot
(e.g., const nameBlank = !bot?.name?.trim()), call setNameMissing(nameBlank) and
if nameBlank return early, then create a shallow copy of the bot (do not assign
to bot directly) and set copy.simple_editor = false before calling
upsertBot(copy); keep existing Sentry error capture and onSwitchEditor(newBot)
flow but operate on the copied object to avoid mutating React state.
In `@front/app/parent/notifications.tsx`:
- Around line 130-147: The async effect handlers handleNotifications and
setupDevice perform awaited API calls (fetchDeviceByToken, upsertDevice,
fetchDevice, etc.) and are invoked fire-and-forget; wrap the bodies of both
async functions in try/catch blocks to catch and handle errors from those
awaits, log or report the error (e.g., console.error or the app logger) with
context about which operation failed, and ensure the catch path either sets a
safe state (e.g., disable notifications) or returns gracefully to avoid
unhandled promise rejections.
In `@front/app/parent/profileEditor.tsx`:
- Around line 50-65: validateProfile currently only updates nameMissing but
doesn't communicate validity; modify validateProfile (or make it return a
boolean) so it returns whether the profile is valid (e.g., false when
profile?.name.trim() is empty) and then update saveProfile to await
validateProfile() and early-return when validation fails before calling
upsertProfile(profile). Refer to validateProfile, saveProfile, upsertProfile,
setNameMissing and profile when making the change.
---
Nitpick comments:
In `@front/app/parent/botEditor.tsx`:
- Around line 20-52: loadSelectedBot can commit stale state when
fetchBot()/fetchAiModels() resolve after botId changes or unmounts; update the
effect to guard commits by either: (A) change loadSelectedBot to return the
fetched bot object (or null) instead of calling setBot internally and inside the
useEffect call loadSelectedBot().then(fetched => { if (!isStale)
setBot(fetched); }) where isStale is a flag set false at start and flipped true
in the effect cleanup, or (B) accept an AbortSignal/flag into loadSelectedBot
and have it check the signal before calling setBot; ensure the useEffect creates
the flag/AbortController, passes it in, and cleans up by marking stale/aborting
so late responses never call setBot. Use identifiers loadSelectedBot, fetchBot,
fetchAiModels, setBot, useEffect, and local.botId to locate the code to modify.
In `@front/components/__tests__/ThemedText-test.tsx`:
- Around line 2-13: The test currently uses react-test-renderer (imports
renderer and act, calls renderer.create(...).toJSON()) which is discouraged for
React 19; replace it with React Native Testing Library: remove imports of
renderer and act, import { render } from '@testing-library/react-native', call
const tree = render(<ThemedText>Snapshot test!</ThemedText>).toJSON() (or const
{ toJSON } = render(...); expect(toJSON()).toMatchSnapshot()), and keep the
assertion on toMatchSnapshot() so the test targets the ThemedText component
using render from the testing library.
In `@front/eslint.config.js`:
- Around line 10-13: The ESLint import resolver 'typescript' referenced under
the 'import/resolver' config is currently satisfied only transitively; add an
explicit devDependency "eslint-import-resolver-typescript" to front/package.json
(devDependencies) so the resolver is guaranteed present; update package.json
accordingly and run npm/yarn install to update lockfile so CI and clean installs
won't rely on transitive packages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95b53e08-2beb-44c3-b4ac-af644cc5f651
⛔ Files ignored due to path filters (1)
front/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
.github/workflows/lint-test.ymlfront/api/account.tsfront/api/apiClient.tsfront/api/bots.tsfront/api/chats.tsfront/api/devices.tsfront/api/profiles.tsfront/app/__tests__/_layout-test.tsxfront/app/_layout.tsxfront/app/botChat.tsxfront/app/chatList.tsxfront/app/login.tsxfront/app/parent/botAdvanced.tsxfront/app/parent/botEditor.tsxfront/app/parent/botSimple.tsxfront/app/parent/botsList.tsxfront/app/parent/deleteAccount.tsxfront/app/parent/notifications.tsxfront/app/parent/profileEditor.tsxfront/app/parent/profilesList.tsxfront/app/parent/setPin.tsxfront/app/parent/settings.tsxfront/app/selectBot.tsxfront/components/AppleSignInButton.tsxfront/components/ChatMessage.tsxfront/components/ErrorBoundary.tsxfront/components/GoogleSignInButton.tsxfront/components/HelloWave.tsxfront/components/MenuItem.tsxfront/components/PinWrapper.tsxfront/components/__tests__/ThemedText-test.tsxfront/eslint.config.jsfront/package.json
💤 Files with no reviewable changes (2)
- .github/workflows/lint-test.yml
- front/app/parent/settings.tsx
✅ Files skipped from review due to trivial changes (14)
- front/api/profiles.ts
- front/app/parent/deleteAccount.tsx
- front/api/chats.ts
- front/api/devices.ts
- front/api/apiClient.ts
- front/api/bots.ts
- front/components/AppleSignInButton.tsx
- front/components/GoogleSignInButton.tsx
- front/app/parent/botsList.tsx
- front/components/PinWrapper.tsx
- front/components/MenuItem.tsx
- front/app/parent/setPin.tsx
- front/app/login.tsx
- front/app/botChat.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- front/components/ChatMessage.tsx
| const { data, status } = await apiClient<Account>(`/user?timezone=${deviceTimeZone}`); | ||
|
|
||
| if (!ok) { | ||
| if (status !== 200) { | ||
| throw new Error(`Failed to fetch account with status ${status}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Locate /user and /user/delete route or contract definitions"
rg -n -C3 --hidden \
-g '!**/node_modules/**' -g '!**/dist/**' \
-e '"/user/delete"|'\''/user/delete'\''' \
-e '"/user"|'\''/user'\'''
echo
echo "2) Locate explicit response statuses near API handlers/specs"
rg -n -C4 --hidden \
-g '!**/node_modules/**' -g '!**/dist/**' \
-e 'status\((200|201|202|204)\)' \
-e 'sendStatus\((200|201|202|204)\)' \
-e 'responses:\s*\{'Repository: tpaulshippy/bots
Length of output: 941
🏁 Script executed:
# Find and examine apiClient.ts to understand ok vs status
find . -name "apiClient.ts" -o -name "apiClient.js" | head -5Repository: tpaulshippy/bots
Length of output: 84
🏁 Script executed:
# Check what apiClient returns and how ok/status are defined
rg -n "ApiResponse|interface.*Response" front/api/ -A 5Repository: tpaulshippy/bots
Length of output: 1148
🏁 Script executed:
# Look for backend code in the repository (common locations)
fd -e "ts" -e "js" -e "json" "route\|server\|handler" --max-depth 3 | grep -E "back|server|route" | head -20Repository: tpaulshippy/bots
Length of output: 42
🏁 Script executed:
# Search for OpenAPI or API specification files
find . -name "*.yaml" -o -name "*.yml" -o -name "*.json" | xargs grep -l "user\|delete" 2>/dev/null | head -10Repository: tpaulshippy/bots
Length of output: 76
🏁 Script executed:
# Check how other API calls in the codebase handle success verification
rg -n "if.*ok\b|if.*status" front/api/ -B 2 -A 1Repository: tpaulshippy/bots
Length of output: 5810
🏁 Script executed:
cat -n front/api/apiClient.ts | head -80Repository: tpaulshippy/bots
Length of output: 2635
Use ok for success checks to match codebase pattern.
Lines 19 and 60 check exact status codes (200 and 204), but the entire codebase—and the ApiResponse interface itself—uses the ok flag for success validation. This exact-match approach is stricter and inconsistent. Line 41 in the same file (updateAccount) already uses !ok, confirming the intended pattern.
Replace both exact status checks with !ok to align with devices.ts, profiles.ts, bots.ts, chats.ts, and aiModels.ts:
Suggested fix
- const { data, status } = await apiClient<Account>(`/user?timezone=${deviceTimeZone}`);
+ const { data, ok, status } = await apiClient<Account>(`/user?timezone=${deviceTimeZone}`);
- if (status !== 200) {
+ if (!ok) {
throw new Error(`Failed to fetch account with status ${status}`);
}
...
- const { status } = await apiClient<void>('/user/delete', {
+ const { ok, status } = await apiClient<void>('/user/delete', {
method: 'DELETE',
});
- if (status !== 204) {
+ if (!ok) {
throw new Error(`Failed to delete account with status ${status}`);
}Also applies to: 56–61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/api/account.ts` around lines 17 - 20, The success checks in the account
API calls currently compare HTTP status codes (e.g., using status === 200 and
status === 204) rather than using the ApiResponse.ok flag used across the
codebase; update the handlers that call apiClient<Account>(...) and
apiClient<void>(...) so they validate response.ok (or !ok) instead of exact
status codes, and throw errors when ok is false while keeping the same error
messages and returned data variables (data, status, ok); reference the apiClient
call sites in this file (the functions that fetch the Account and the one that
updates the account) and follow the pattern used in updateAccount where !ok is
already used.
| const receivedSubscription = (Notifications.addNotificationReceivedListener as jest.Mock).mock.results[0]?.value; | ||
| const responseSubscription = (Notifications.addNotificationResponseReceivedListener as jest.Mock).mock.results[0]?.value; | ||
|
|
||
| expect(receivedSubscription.remove).toHaveBeenCalled(); | ||
| expect(responseSubscription.remove).toHaveBeenCalled(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the test file structure and the specific lines mentioned
cd front/app/__tests__ && cat -n _layout-test.tsx | head -30Repository: tpaulshippy/bots
Length of output: 1182
🏁 Script executed:
# Now let's look at lines 15-25 to see the mock setup
cat -n _layout-test.tsx | sed -n '15,25p'Repository: tpaulshippy/bots
Length of output: 108
🏁 Script executed:
# Look at lines 170-190 to see the full context of the assertions
cat -n _layout-test.tsx | sed -n '170,190p'Repository: tpaulshippy/bots
Length of output: 108
🏁 Script executed:
# Let's also check the actual implementation to understand cleanup behavior
cat -n ../../../app/_layout.tsx | sed -n '95,130p'Repository: tpaulshippy/bots
Length of output: 116
🏁 Script executed:
# Check if the test is skipped (using "skip" or "pending" keywords)
grep -n "\.skip\|\.todo\|pending\|xit" _layout-test.tsx | head -20Repository: tpaulshippy/bots
Length of output: 109
🏁 Script executed:
fd -type f -name "_layout-test.tsx"Repository: tpaulshippy/bots
Length of output: 230
🏁 Script executed:
fd -type f -name "_layout.tsx" | grep appRepository: tpaulshippy/bots
Length of output: 230
🏁 Script executed:
fd -t f -name "_layout-test.tsx"Repository: tpaulshippy/bots
Length of output: 290
🏁 Script executed:
fd -t f -name "_layout.tsx" | grep appRepository: tpaulshippy/bots
Length of output: 290
🏁 Script executed:
fd "_layout-test.tsx"Repository: tpaulshippy/bots
Length of output: 96
🏁 Script executed:
fd "_layout.tsx"Repository: tpaulshippy/bots
Length of output: 81
🏁 Script executed:
cat -n front/app/__tests__/_layout-test.tsx | sed -n '170,195p'Repository: tpaulshippy/bots
Length of output: 857
🏁 Script executed:
wc -l front/app/_layout.tsxRepository: tpaulshippy/bots
Length of output: 85
🏁 Script executed:
cat -n front/app/_layout.tsx | sed -n '90,140p'Repository: tpaulshippy/bots
Length of output: 1882
🏁 Script executed:
# Check the entire test to understand the render call and mock.results
cat -n front/app/__tests__/_layout-test.tsx | sed -n '150,185p'Repository: tpaulshippy/bots
Length of output: 1600
This assertion never exercises the real cleanup path.
The test is currently skipped (line 172). Because ../_layout is mocked to return null on lines 19-21, render(<RootLayout />) never executes the real useEffect that registers notification listeners (lines 100-130 in _layout.tsx). Once this test is enabled, both mock.results[0] lookups will be undefined, causing the .remove() calls on lines 183-184 to fail. Remove the jest.mock('../_layout', ...) or seed the Notifications mocks with subscription objects before rendering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/app/__tests__/_layout-test.tsx` around lines 180 - 184, The test fails
to exercise the real cleanup because ../_layout is mocked to return null so
RootLayout's useEffect never registers notification listeners; either remove the
jest.mock('../_layout', ...) so render(<RootLayout />) runs the real useEffect
(which registers via Notifications.addNotificationReceivedListener and
Notifications.addNotificationResponseReceivedListener) or, if you must keep the
mock, seed the Notifications.addNotificationReceivedListener and
Notifications.addNotificationResponseReceivedListener mocks with fake
subscription objects that have a remove method before calling render so
(Notifications.addNotificationReceivedListener as
jest.Mock).mock.results[0]?.value and
(Notifications.addNotificationResponseReceivedListener as
jest.Mock).mock.results[0]?.value are defined and their .remove() calls can be
asserted.
| useEffect(() => { | ||
| if (loaded) { | ||
| const subscription = Linking.addEventListener("url", getJWTFromLink); | ||
|
|
||
| const initialize = async () => { | ||
| SplashScreen.hideAsync(); | ||
| Linking.addEventListener("url", getJWTFromLink); | ||
| await initialNavigationChecks(); | ||
| }; | ||
|
|
||
| initialize(); | ||
| void initialize(); | ||
|
|
||
| return () => { | ||
| subscription.remove(); | ||
| }; | ||
| } | ||
| }, [loaded]); | ||
| }, [getJWTFromLink, initialNavigationChecks, loaded]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the actual code to see what's at lines 181-196
cat -n front/app/_layout.tsx | sed -n '175,205p'Repository: tpaulshippy/bots
Length of output: 957
🏁 Script executed:
# Check if Linking.getInitialURL is used anywhere in the codebase
rg "getInitialURL" front/Repository: tpaulshippy/bots
Length of output: 42
🏁 Script executed:
# Look at the entire _layout.tsx file to understand the context
wc -l front/app/_layout.tsxRepository: tpaulshippy/bots
Length of output: 85
🏁 Script executed:
# Let's search for getJWTFromLink definition
rg "getJWTFromLink" front/app/_layout.tsx -B 5 -A 10Repository: tpaulshippy/bots
Length of output: 1188
🏁 Script executed:
# Check if there's any deep link handling in app initialization
rg "Linking\." front/app/_layout.tsxRepository: tpaulshippy/bots
Length of output: 185
🏁 Script executed:
# Look for any other Expo deep link handling patterns in the codebase
rg "Linking\.getInitialURL|Linking\.addEventListener" front/Repository: tpaulshippy/bots
Length of output: 157
🏁 Script executed:
# Let's see the full _layout.tsx file to understand the overall structure
cat -n front/app/_layout.tsx | head -100Repository: tpaulshippy/bots
Length of output: 3902
🏁 Script executed:
# Let's see more of the file, particularly the getJWTFromLink function definition
cat -n front/app/_layout.tsx | sed -n '100,180p'Repository: tpaulshippy/bots
Length of output: 3031
🏁 Script executed:
# Check Expo documentation for Linking API best practices by searching for any comments or setup
rg "Linking|deep.*link" front/app/_layout.tsx -iRepository: tpaulshippy/bots
Length of output: 226
🏁 Script executed:
# Let's verify the Expo Linking API pattern by checking if there are any other files that handle deep linking
rg "getInitialURL" . --type ts --type tsxRepository: tpaulshippy/bots
Length of output: 87
🏁 Script executed:
# Check the app.json or app configuration to see if deep linking is configured
fd -e "json" -e "config" | xargs grep -l "scheme\|deep.*link\|linking" 2>/dev/null | head -10Repository: tpaulshippy/bots
Length of output: 117
🏁 Script executed:
# Check the app.json for deep linking configuration
cat front/app.jsonRepository: tpaulshippy/bots
Length of output: 2118
🏁 Script executed:
# Search for deep linking patterns in the entire front directory
rg "getInitialURL|deeplink|deep.*link" front/ -iRepository: tpaulshippy/bots
Length of output: 42
Process the initial deep link on cold start, not just runtime events.
This effect only subscribes to future "url" events via addEventListener. When the app is cold-started from a deep link (e.g., clicking an auth email link), Linking.getInitialURL() must be called to process that initial URL—otherwise, getJWTFromLink never runs and the token handoff/login flow fails.
Suggested fix
useEffect(() => {
if (loaded) {
const subscription = Linking.addEventListener("url", getJWTFromLink);
const initialize = async () => {
+ const initialUrl = await Linking.getInitialURL();
+ if (initialUrl) {
+ await getJWTFromLink({ url: initialUrl });
+ }
await initialNavigationChecks();
- SplashScreen.hideAsync();
+ await SplashScreen.hideAsync();
};
void initialize();
return () => {
subscription.remove();
};
}
}, [getJWTFromLink, initialNavigationChecks, loaded]);📝 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.
| useEffect(() => { | |
| if (loaded) { | |
| const subscription = Linking.addEventListener("url", getJWTFromLink); | |
| const initialize = async () => { | |
| SplashScreen.hideAsync(); | |
| Linking.addEventListener("url", getJWTFromLink); | |
| await initialNavigationChecks(); | |
| }; | |
| initialize(); | |
| void initialize(); | |
| return () => { | |
| subscription.remove(); | |
| }; | |
| } | |
| }, [loaded]); | |
| }, [getJWTFromLink, initialNavigationChecks, loaded]); | |
| useEffect(() => { | |
| if (loaded) { | |
| const subscription = Linking.addEventListener("url", getJWTFromLink); | |
| const initialize = async () => { | |
| const initialUrl = await Linking.getInitialURL(); | |
| if (initialUrl) { | |
| await getJWTFromLink({ url: initialUrl }); | |
| } | |
| await initialNavigationChecks(); | |
| await SplashScreen.hideAsync(); | |
| }; | |
| void initialize(); | |
| return () => { | |
| subscription.remove(); | |
| }; | |
| } | |
| }, [getJWTFromLink, initialNavigationChecks, loaded]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/app/_layout.tsx` around lines 181 - 196, The effect currently only
subscribes to future Linking "url" events and misses processing a cold-start
deep link; update the useEffect that references getJWTFromLink,
initialNavigationChecks, SplashScreen.hideAsync and loaded to also call
Linking.getInitialURL() once during initialization (before or as part of the
initialize async function) and, if it returns a URL, invoke getJWTFromLink with
that URL (handle it the same way the event handler does), then proceed to
addEventListener as before; ensure the initialize function awaits
SplashScreen.hideAsync(), calls initialNavigationChecks(), safely handles errors
from getInitialURL(), and still returns the subscription.remove() cleanup.
| } else if (deviceRef.current) { | ||
| const updatedDevice = { | ||
| ...deviceRef.current, | ||
| notify_on_new_chat: false, | ||
| notify_on_new_message: false, | ||
| }; | ||
| await upsertDevice(updatedDevice); | ||
| updateDeviceState(updatedDevice); | ||
| } |
There was a problem hiding this comment.
Don’t update local device ref when backend persistence fails.
At Line 125, upsertDevice(updatedDevice) may return null; Line 126 still writes updatedDevice into deviceRef, which can desync local state from server state.
Suggested fix
- await upsertDevice(updatedDevice);
- updateDeviceState(updatedDevice);
+ const persistedDevice = await upsertDevice(updatedDevice);
+ if (persistedDevice) {
+ updateDeviceState(persistedDevice);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/app/parent/notifications.tsx` around lines 119 - 127, The code updates
the local deviceRef and calls updateDeviceState with updatedDevice even if
upsertDevice(updatedDevice) fails or returns null, which can desync client state
from the server; change the flow in the block that constructs updatedDevice (the
object with notify_on_new_chat and notify_on_new_message) to await
upsertDevice(updatedDevice), check the returned value (the upsert result) and
only call updateDeviceState and write to deviceRef.current when the upsert
returned a non-null/valid device; if upsertDevice returns null or throws,
preserve the existing deviceRef.current and handle/log the error instead of
applying updatedDevice locally.
| componentDidCatch(error: Error) { | ||
| Sentry.captureException(error); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n front/components/ErrorBoundary.tsxRepository: tpaulshippy/bots
Length of output: 1831
🏁 Script executed:
rg "Sentry\." front --type ts --type tsx -A 2 -B 1Repository: tpaulshippy/bots
Length of output: 87
🏁 Script executed:
web_search
Does Sentry.captureException support extra context parameters?Repository: tpaulshippy/bots
Length of output: 153
🏁 Script executed:
rg "Sentry\." front -A 2 -B 1 --type-add "tsx:*.tsx" --type-add "ts:*.ts"Repository: tpaulshippy/bots
Length of output: 6920
🏁 Script executed:
rg "componentDidCatch\|ErrorInfo" front --type-add "tsx:*.tsx" --type-add "ts:*.ts"Repository: tpaulshippy/bots
Length of output: 42
🌐 Web query:
Sentry captureException React Native errorInfo context parameters
💡 Result:
In Sentry for React Native, Sentry.captureException(exception, hint) accepts a second optional parameter called hint (also referred to as captureContext in some contexts). This hint can be: 1. A plain object with specific keys: tags, extra, contexts, user, level, fingerprint. These merge with the current scope data. Example: Sentry.captureException(new Error("something went wrong"), { tags: { section: "articles" }, contexts: { formContext: { step: 1 } }, }); 2. A scope callback function: (scope) => { ...; return scope; } to modify the scope for this event. Example: Sentry.captureException(error, (scope) => { scope.setTag("section", "articles"); scope.setContext("params", { foo: "bar" }); return scope; }); 3. A Scope instance to extract attributes from. This data enriches the captured event with context parameters. For React error boundaries (componentDidCatch(error, errorInfo)), use Sentry.captureException(error) directly (React Native SDK does not have captureReactException like web React SDK). Add errorInfo.componentStack as extra context: componentDidCatch(error, errorInfo) { Sentry.captureException(error, { extra: { componentStack: errorInfo.componentStack } }); } Or use Sentry.ErrorBoundary component, which handles capture automatically and supports beforeCapture prop for adding context. Global context via Sentry.setContext("name", { ... }) or scope methods also applies to captured events. Note: Some GitHub issues mention occasional overwriting by native scope sync, but official docs confirm support for per-capture context.
Citations:
- 1: https://docs.sentry.io/platforms/react-native/enriching-events/context/
- 2: https://docs.sentry.io/platforms/react-native/enriching-events/context.md
- 3: https://docs.sentry.io/platforms/react-native/integrations/error-boundary.md
- 4: https://docs.sentry.io/platforms/javascript/guides/react/features/error-boundary.md
- 5: https://docs.sentry.io/platforms/javascript/guides/react/usage.md
Capture component stack context in boundary error logging.
componentDidCatch receives errorInfo containing componentStack which is essential for debugging root-level failures. Without it, Sentry loses the React component hierarchy when the app-wide error boundary catches unhandled UI errors, making production debugging significantly harder.
Suggested fix
- componentDidCatch(error: Error) {
- Sentry.captureException(error);
+ componentDidCatch(error: Error, errorInfo: React.ErrorInfo) {
+ Sentry.captureException(error, {
+ extra: {
+ componentStack: errorInfo.componentStack,
+ },
+ });
}📝 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.
| componentDidCatch(error: Error) { | |
| Sentry.captureException(error); | |
| componentDidCatch(error: Error, errorInfo: React.ErrorInfo) { | |
| Sentry.captureException(error, { | |
| extra: { | |
| componentStack: errorInfo.componentStack, | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/components/ErrorBoundary.tsx` around lines 26 - 27, componentDidCatch
currently only captures the Error object and omits React's errorInfo
(componentStack); update ErrorBoundary.componentDidCatch to accept the second
parameter (errorInfo) and include the componentStack when reporting to
Sentry—either pass errorInfo (or errorInfo.componentStack) as
Sentry.captureException's second argument or use Sentry.withScope to add
errorInfo.componentStack to the event extras before calling
Sentry.captureException to preserve the React component hierarchy for debugging.
There was a problem hiding this comment.
Pull request overview
Adds rich rendering for assistant chat messages and updates the frontend/tooling to support the new rendering approach (alongside a broader Expo/RN dependency upgrade).
Changes:
- Introduces
MarkdownRendererand switches assistant chat messages to render markdown (user messages remain plain/selectable). - Upgrades Expo/RN ecosystem dependencies and adjusts Jest/lint/CI configuration accordingly.
- Refactors various screens/components for hook dependency correctness and listener cleanup.
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| front/package.json | Updates lint command, Jest transform settings, and upgrades/adds dependencies for markdown/math rendering and Expo 55 stack. |
| front/eslint.config.js | Switches import resolver to TypeScript project-based resolution; adds Jest globals for setup file. |
| front/components/tests/ThemedText-test.tsx | Wraps snapshot creation in act() to align with async rendering expectations. |
| front/components/PinWrapper.tsx | Removes unused imports (cleanup). |
| front/components/MenuItem.tsx | Removes unused theme/color imports and variables (cleanup). |
| front/components/MarkdownRenderer.tsx | New component for themed Markdown rendering and link handling. |
| front/components/HelloWave.tsx | Fixes hook dependency array for animation callback. |
| front/components/GoogleSignInButton.tsx | Removes unused imports (cleanup). |
| front/components/ErrorBoundary.tsx | Adjusts imports and error boundary capture method signature. |
| front/components/ChatMessage.tsx | Uses MarkdownRenderer for assistant messages; keeps user messages as plain selectable text. |
| front/components/AppleSignInButton.tsx | Removes unused imports (cleanup). |
| front/app/selectBot.tsx | Fixes hook dependency array. |
| front/app/parent/settings.tsx | Removes unused import (cleanup). |
| front/app/parent/setPin.tsx | Reorders/imports and fixes hook dependency array. |
| front/app/parent/profilesList.tsx | Refactors callbacks/dependencies; cleans unused imports and handler signature. |
| front/app/parent/profileEditor.tsx | Memoizes async helpers and fixes effect dependencies. |
| front/app/parent/notifications.tsx | Refactors device state handling and async effects; removes unused local param handling. |
| front/app/parent/deleteAccount.tsx | Simplifies catch clause. |
| front/app/parent/botsList.tsx | Cleans function signature. |
| front/app/parent/botSimple.tsx | Refactors validation/editor switching and system prompt updates; removes unused imports/state. |
| front/app/parent/botEditor.tsx | Memoizes bot loading and fixes effect dependencies. |
| front/app/parent/botAdvanced.tsx | Memoizes validation/save callbacks; tightens equality checks. |
| front/app/login.tsx | Removes unused imports and pin-clear helper. |
| front/app/chatList.tsx | Removes debounce usage; refactors refresh/reset logic with callbacks and strict equality. |
| front/app/botChat.tsx | Refactors refresh into callback, fixes effect deps, removes fixed-height getItemLayout. |
| front/app/_layout.tsx | Cleans up notification subscriptions via .remove(), memoizes navigation helpers, and manages deep-link subscription lifecycle. |
| front/app/tests/_layout-test.tsx | Reorders imports/mocks and updates assertions to match subscription .remove() cleanup. |
| front/app.json | Updates Expo app config (status bar + plugin list changes). |
| front/api/profiles.ts | Uses strict equality for sentinel ID check. |
| front/api/devices.ts | Uses strict equality for sentinel ID check. |
| front/api/chats.ts | Consolidates imports from apiClient. |
| front/api/bots.ts | Uses strict equality for sentinel ID check. |
| front/api/apiClient.ts | Removes unused clearUser import. |
| front/api/account.ts | Changes response handling to use status checks; removes unused ok destructuring. |
| .github/workflows/lint-test.yml | Uses npm ci --legacy-peer-deps for CI dependency installation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onLinkPress={(url: string) => { | ||
| Linking.openURL(url).catch(() => null); | ||
| return true; | ||
| }} |
There was a problem hiding this comment.
onLinkPress opens any URL from assistant-provided Markdown without validation. Since assistant content can be untrusted, it’s safer to restrict to allowed schemes (e.g., http/https) and/or call Linking.canOpenURL before opening, otherwise file:, intent:, or other deep-link schemes could be triggered.
| ) : ( | ||
| <ThemedView style={styles.assistantMessage(assistantColor)}> | ||
| <MarkdownRenderer content={message.text} /> | ||
| </ThemedView> |
There was a problem hiding this comment.
Assistant messages were previously rendered with <ThemedText selectable />, but assistant output is now rendered via MarkdownRenderer without any selection/copy support. If users need to copy assistant responses, consider enabling text selection in the renderer or adding a copy action to avoid a UX regression.
| <ThemedView style={styles.assistantMessage(assistantColor)}> | ||
| <MarkdownRenderer content={message.text} /> | ||
| </ThemedView> |
There was a problem hiding this comment.
New assistant-message rendering uses MarkdownRenderer, but there’s no test coverage verifying this role-based branching (assistant renders markdown, user remains plain/selectable) or markdown normalization. Adding a minimal test around this component would help prevent regressions in the core chat UI.
| componentDidCatch(error: Error) { | ||
| Sentry.captureException(error); |
There was a problem hiding this comment.
componentDidCatch dropped the errorInfo argument. React provides errorInfo.componentStack, which is useful context for Sentry reports; consider keeping the second parameter (even as _errorInfo) and attaching it to the Sentry event (e.g., via captureException scope/extras) to avoid losing diagnostics.
| componentDidCatch(error: Error) { | |
| Sentry.captureException(error); | |
| componentDidCatch(error: Error, errorInfo: React.ErrorInfo) { | |
| Sentry.captureException(error, (scope) => { | |
| scope.setExtra('componentStack', errorInfo.componentStack); | |
| return scope; | |
| }); |
| "katex": "^0.16.45", | ||
| "marked": "^18.0.0", |
There was a problem hiding this comment.
marked and katex are added as dependencies, but the current frontend implementation doesn’t reference either library (Markdown rendering uses react-native-markdown-display). If they’re not needed, remove them to reduce bundle size/install time; if they are intended for WebView/KaTeX rendering, wire them into the implementation.
| "transformIgnorePatterns": [ | ||
| "node_modules/(?!((jest-)?react-native|@react-native(-community)?)|expo(nent)?|@expo(nent)?/.*|@expo-google-fonts/.*|react-navigation|@react-navigation/.*|@unimodules/.*|unimodules|sentry-expo|native-base|react-native-svg)" | ||
| "node_modules/(?!((jest-)?react-native|@react-native(-community)?)|expo(nent)?|@expo(nent)?/.*|@expo-google-fonts/.*|react-navigation|@react-navigation/.*|@unimodules/.*|unimodules|sentry-expo|native-base|react-native-svg|marked|katex)" | ||
| ] |
There was a problem hiding this comment.
Jest transformIgnorePatterns was expanded to include marked|katex, but the codebase doesn’t currently import these packages. If marked/katex are removed, revert this change as well; otherwise add a short note or a test that exercises these deps so the custom transform configuration stays justified.
| import { useColorScheme } from '@/hooks/useColorScheme'; | ||
| import Markdown from 'react-native-markdown-display'; | ||
|
|
||
| interface MarkdownRendererProps { |
There was a problem hiding this comment.
MarkdownRenderer currently uses react-native-markdown-display only and does not implement any KaTeX/LaTeX rendering. As a result, inline math like \(a=54.5\) will render as plain text, which conflicts with the PR goal/description. Either implement math parsing + KaTeX rendering here (e.g., custom rule that detects \(...\)/\[...\] and renders via KaTeX/WebView) or update the PR description and remove the unused math deps.
| return content | ||
| .replace(/\r\n/g, '\n') | ||
| .replace(/\n{3,}/g, '\n\n') | ||
| .trim(); |
There was a problem hiding this comment.
normalizeMarkdown() calls .trim(), which removes leading whitespace/newlines and can change Markdown semantics (e.g., indented code blocks or content that intentionally starts with whitespace). Consider removing the trim or only trimming trailing newlines while preserving leading whitespace.
| .trim(); | |
| .replace(/\n+$/, ''); |
tpaulshippy
changed the title
Summary
ChatMessage.tsxto use MarkdownRenderer for assistant messages while keeping plain text for user messagesTesting
Test with messages containing:
code, lists, links, blockquotesSummary by CodeRabbit
New Features
Chores