🧪 QA: Comprehensive Maestro E2E test suite (25 flows, 25/25 passing)#1867
🧪 QA: Comprehensive Maestro E2E test suite (25 flows, 25/25 passing)#1867andrew-bierman wants to merge 0 commit into
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:
📝 WalkthroughWalkthroughAdds a new Offline AI feature (types, mock LLM provider, hook, UI, screen, tests, README) and updates dependency configuration: preinstall script now writes a project Changes
Sequence Diagram(s)sequenceDiagram
participant UI as OfflineAIChat
participant Hook as useOfflineAI
participant LLM as MockLLMProvider
participant DB as TRAIL_DATABASE
UI->>Hook: initialize(config?)
Hook->>LLM: initialize(config)
LLM-->>Hook: ready + modelInfo
UI->>Hook: sendMessage(content, trailContext?)
Hook->>Hook: append user message
Hook->>LLM: generate(prompt, TrailQAContext)
LLM->>DB: lookup trail (id/name)
DB-->>LLM: trail data
LLM-->>Hook: assistant response
Hook->>UI: append assistant message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🧹 Nitpick comments (2)
apps/expo/features/offline-ai/lib/mock-llm-provider.ts (1)
290-291: Exported singleton is not pre-initialized.The
defaultLLMProvidersingleton is created but not initialized. Consumers must callinitialize()beforegenerate(), otherwise they'll get an error. This may be intentional, but consider documenting this requirement or providing a pre-initialized variant for convenience in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/offline-ai/lib/mock-llm-provider.ts` around lines 290 - 291, The exported singleton defaultLLMProvider is created via createLLMProvider() but not initialized, so callers must call initialize() before using generate(); update the export to provide a pre-initialized variant (or an explicitly named initializedDefaultLLMProvider) by calling initialize() on the created provider before export, or alternatively document the requirement clearly in the module README/export comment; reference createLLMProvider(), defaultLLMProvider, initialize(), and generate() so you initialize the instance or expose a clearly named factory and an initialized instance for tests..github/scripts/configure-deps.ts (1)
86-91: Consider adding guard against accidentally overwriting existing.npmrcwith custom configurations.If a developer has a pre-existing
.npmrcwith additional npm configurations (e.g., custom registries for other scopes), this function will overwrite it entirely. Consider appending/updating only the GitHub Packages line, or warning if a user-maintained.npmrcexists.💡 Optional enhancement to preserve existing .npmrc entries
+import { readFileSync, existsSync } from 'node:fs'; + function writeNpmrc(token: string) { const projectRoot = resolve(import.meta.dir, '..', '..'); const npmrcPath = resolve(projectRoot, '.npmrc'); - const content = `${NPMRC_AUTH_LINE}${token}\n`; + const newLine = `${NPMRC_AUTH_LINE}${token}`; + + let content = ''; + if (existsSync(npmrcPath)) { + const existing = readFileSync(npmrcPath, 'utf-8'); + // Remove old GitHub Packages auth line if present + const lines = existing.split('\n').filter(line => !line.startsWith(NPMRC_AUTH_LINE)); + content = lines.filter(Boolean).join('\n'); + if (content) content += '\n'; + } + content += newLine + '\n'; writeFileSync(npmrcPath, content, { mode: 0o600 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/configure-deps.ts around lines 86 - 91, The writeNpmrc function currently overwrites any existing .npmrc; change it to first check for an existing file at npmrcPath (use existsSync), read its contents (readFileSync), and if a line starting with NPMRC_AUTH_LINE (or the same auth entry) already exists replace that line, otherwise append the auth line to the existing content; if the file doesn't exist create it with the auth line and the same 0o600 mode. Also consider emitting a warning (or creating a .npmrc.backup) when you detect a pre-existing file before modifying it so user configurations are preserved. Ensure you update only via the writeNpmrc function and reference NPMRC_AUTH_LINE and npmrcPath when implementing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/features/offline-ai/lib/mock-llm-provider.ts`:
- Around line 282-288: Update the misleading console.warn in the block that
returns new MockLLMProvider so it clearly states the real provider is not
implemented; replace the message that says "Set type to 'real' for actual LLM"
with something like "[OfflineAI] Real LLM provider not implemented yet; using
MockLLMProvider despite type === 'real'." Make this change where MockLLMProvider
is instantiated and the existing console.warn is emitted so logs accurately
reflect that the mock is used because the real implementation is missing.
---
Nitpick comments:
In @.github/scripts/configure-deps.ts:
- Around line 86-91: The writeNpmrc function currently overwrites any existing
.npmrc; change it to first check for an existing file at npmrcPath (use
existsSync), read its contents (readFileSync), and if a line starting with
NPMRC_AUTH_LINE (or the same auth entry) already exists replace that line,
otherwise append the auth line to the existing content; if the file doesn't
exist create it with the auth line and the same 0o600 mode. Also consider
emitting a warning (or creating a .npmrc.backup) when you detect a pre-existing
file before modifying it so user configurations are preserved. Ensure you update
only via the writeNpmrc function and reference NPMRC_AUTH_LINE and npmrcPath
when implementing.
In `@apps/expo/features/offline-ai/lib/mock-llm-provider.ts`:
- Around line 290-291: The exported singleton defaultLLMProvider is created via
createLLMProvider() but not initialized, so callers must call initialize()
before using generate(); update the export to provide a pre-initialized variant
(or an explicitly named initializedDefaultLLMProvider) by calling initialize()
on the created provider before export, or alternatively document the requirement
clearly in the module README/export comment; reference createLLMProvider(),
defaultLLMProvider, initialize(), and generate() so you initialize the instance
or expose a clearly named factory and an initialized instance for tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d8df9aa-2aa0-43a0-b42c-9a38efea25fb
📒 Files selected for processing (4)
.github/scripts/configure-deps.ts.gitignoreapps/expo/features/offline-ai/lib/mock-llm-provider.tsbunfig.toml
| // For future real implementation: | ||
| // import { LlamaCppProvider } from './llama-cpp-provider'; | ||
| // return new LlamaCppProvider(); | ||
|
|
||
| console.warn('[OfflineAI] Using mock provider. Set type to "real" for actual LLM.'); | ||
| return new MockLLMProvider(); | ||
| } |
There was a problem hiding this comment.
Warning message is misleading.
The warning at line 286 says "Set type to 'real' for actual LLM" but this code path is reached precisely when type === 'real'. The message should indicate that real implementation is not yet available.
🐛 Proposed fix for the warning message
// For future real implementation:
// import { LlamaCppProvider } from './llama-cpp-provider';
// return new LlamaCppProvider();
- console.warn('[OfflineAI] Using mock provider. Set type to "real" for actual LLM.');
+ console.warn('[OfflineAI] Real LLM provider not yet implemented. Falling back to mock provider.');
return new MockLLMProvider();📝 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.
| // For future real implementation: | |
| // import { LlamaCppProvider } from './llama-cpp-provider'; | |
| // return new LlamaCppProvider(); | |
| console.warn('[OfflineAI] Using mock provider. Set type to "real" for actual LLM.'); | |
| return new MockLLMProvider(); | |
| } | |
| // For future real implementation: | |
| // import { LlamaCppProvider } from './llama-cpp-provider'; | |
| // return new LlamaCppProvider(); | |
| console.warn('[OfflineAI] Real LLM provider not yet implemented. Falling back to mock provider.'); | |
| return new MockLLMProvider(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/offline-ai/lib/mock-llm-provider.ts` around lines 282 -
288, Update the misleading console.warn in the block that returns new
MockLLMProvider so it clearly states the real provider is not implemented;
replace the message that says "Set type to 'real' for actual LLM" with something
like "[OfflineAI] Real LLM provider not implemented yet; using MockLLMProvider
despite type === 'real'." Make this change where MockLLMProvider is instantiated
and the existing console.warn is emitted so logs accurately reflect that the
mock is used because the real implementation is missing.
There was a problem hiding this comment.
Pull request overview
This PR improves developer/CI dependency installation for private GitHub Packages by shifting auth to a generated .npmrc, and adds a mock LLM provider intended to support offline AI testing in the Expo app.
Changes:
- Remove inline token usage from
bunfig.tomlfor the@packrat-aiinstall scope. - Update the preinstall dependency configuration script to write a repo-root
.npmrcwith a GitHub Packages auth token (and gitignore it). - Add
MockLLMProviderto generate trail-context-aware responses for offline AI scenarios.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| bunfig.toml | Removes inline token configuration for the @packrat-ai scope, relying on external auth configuration. |
| .github/scripts/configure-deps.ts | Writes .npmrc with GitHub Packages auth token during preinstall (local + CI). |
| .gitignore | Ignores .npmrc to prevent committing tokens. |
| apps/expo/features/offline-ai/lib/mock-llm-provider.ts | Adds a mock LLM provider with context-aware response generation logic. |
You can also share your feedback on Copilot code review. Take the survey.
| import type { LLMConfig, LLMProvider, TrailInfo, TrailQAContext } from '../types'; | ||
|
|
There was a problem hiding this comment.
The import from '../types' appears to point to apps/expo/features/offline-ai/types, but that path/file doesn’t exist in the repo. This will break TypeScript builds; either add the missing types module or update the import to the correct location.
| import type { LLMConfig, LLMProvider, TrailInfo, TrailQAContext } from '../types'; | |
| // Local type definitions to avoid importing from a non-existent '../types' module. | |
| export type LLMConfig = any; | |
| export type LLMProvider = any; | |
| export interface TrailInfo { | |
| id: string; | |
| name: string; | |
| location: string; | |
| difficulty: string; | |
| length: string; | |
| elevation: string; | |
| description: string; | |
| highlights: string[]; | |
| permits: string[]; | |
| hazards: string[]; | |
| bestSeasons: string[]; | |
| } | |
| export type TrailQAContext = any; |
| // In CI, the env var is set before install so bunfig.toml can read it directly | ||
| if (!process.env.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN) { | ||
| console.error('❌ PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN not found in CI'); | ||
| console.error('PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN not found in CI'); |
There was a problem hiding this comment.
This comment is now misleading: bunfig.toml no longer reads PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN directly (the token is written to .npmrc instead). Please update the comment to reflect the new auth flow to avoid confusion when debugging CI installs.
| function writeNpmrc(token: string) { | ||
| const projectRoot = resolve(import.meta.dir, '..', '..'); | ||
| const npmrcPath = resolve(projectRoot, '.npmrc'); | ||
| const content = `${NPMRC_AUTH_LINE}${token}\n`; | ||
| writeFileSync(npmrcPath, content, { mode: 0o600 }); | ||
| } |
There was a problem hiding this comment.
writeNpmrc overwrites any existing repo-root .npmrc, which could clobber developer/project npm settings unrelated to GitHub auth. Consider preserving existing contents and only adding/updating the npm.pkg.github.com auth line (or writing to a dedicated filename and pointing Bun/npm at it).
| if (!process.env.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN) { | ||
| console.error('❌ PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN not found in CI'); | ||
| console.error('PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN not found in CI'); | ||
| console.error('Please ensure PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN is set in your CI secrets'); | ||
| process.exit(1); | ||
| } | ||
| console.log('✓ Using PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN for CI authentication'); | ||
| writeNpmrc(process.env.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN); | ||
| console.log('Using PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN for CI authentication'); | ||
| return; | ||
| } | ||
|
|
||
| // For local development, check if PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN is already set | ||
| if (process.env.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN) { | ||
| console.log('✓ PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN already set in environment'); | ||
| writeNpmrc(process.env.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN); | ||
| console.log('PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN already set in environment'); |
There was a problem hiding this comment.
In the CI and env-var branches, the token is written to .npmrc without trimming/validating. If the secret contains trailing whitespace/newlines (common when copy/pasting), installs can still 401 but the script will report success. Consider normalizing (trim) and erroring on an empty token before writing.
|
|
Good question! The
So the flow is:
We could remove the |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/expo/features/offline-ai/screens/OfflineAIScreen.tsx (1)
13-32: Make demo trail data injectable for non-demo usage.Hardcoding
DEMO_TRAILin the screen limits reuse for real trail flows.Suggested change
+interface OfflineAIScreenProps { + trail?: TrailInfo; +} + -export function OfflineAIScreen() { +export function OfflineAIScreen({ trail = DEMO_TRAIL }: OfflineAIScreenProps) { return ( <SafeAreaView style={styles.container}> <StatusBar barStyle="dark-content" /> - <OfflineAIChat trail={DEMO_TRAIL} showModelInfo={true} /> + <OfflineAIChat trail={trail} showModelInfo={true} /> </SafeAreaView> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/offline-ai/screens/OfflineAIScreen.tsx` around lines 13 - 32, The screen currently hardcodes DEMO_TRAIL inside OfflineAIScreen which prevents passing real trail data; update OfflineAIScreen to accept an external trail (via props or navigation/route params) and pass that into OfflineAIChat, using DEMO_TRAIL only as a fallback when no trail is provided; locate DEMO_TRAIL, the OfflineAIScreen function and the OfflineAIChat usage to implement the prop or route param read and fallback logic so the screen can be reused for non-demo flows.vitest.config.ts (1)
6-6: Expand test include globs to cover.test.tsxfor future component tests.The current patterns only match
.test.ts. While no component tests exist yet, including{ts,tsx}patterns would support component testing if added later (e.g., inapps/expo/components/).Suggested change
- include: ['**/__tests__/**/*.test.ts', '**/*.test.ts'], + include: ['**/__tests__/**/*.test.{ts,tsx}', '**/*.test.{ts,tsx}'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.config.ts` at line 6, Update the vitest include globs so they also match .test.tsx files: modify the include array (the include property in vitest.config.ts) to add patterns that cover .test.tsx (e.g., expand '**/*.test.ts' and '**/__tests__/**/*.test.ts' to include tsx or use a {ts,tsx} variant) so future component tests under apps/expo/components/ will be discovered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/features/offline-ai/components/OfflineAIChat.tsx`:
- Around line 201-382: The file defines a large styles object (styles created
via StyleSheet.create) and uses it throughout OfflineAIChat; replace that with
NativeWind/Tailwind className strings: remove the styles constant and any
StyleSheet import, convert each usage of styles.container, styles.header,
styles.title, modelInfo, initButton, trailBadge, errorContainer,
messagesContainer, messagesContent, emptyState, quickPrompts, quickPromptButton,
messageBubble, userBubble, assistantBubble, messageText, userText,
assistantText, timestamp, typingIndicator, inputContainer, input, sendButton,
sendButtonDisabled, clearButton, etc., into equivalent Tailwind className values
(e.g., styles.container -> "flex-1 bg-gray-50", header -> "p-4 bg-white border-b
border-gray-200 items-center", title -> "text-lg font-extrabold text-gray-900",
etc.) and update the JSX elements (View, Text, ScrollView, TextInput,
TouchableOpacity) to use the className prop provided by `@packrat/ui/nativewindui`
(or nativewind) instead of style={styles.xxx}; ensure conditional styles like
sendButtonDisabled are applied by composing className strings conditionally;
keep all existing component/prop names (e.g., OfflineAIChat, any handlers)
unchanged.
In `@apps/expo/features/offline-ai/hooks/useOfflineAI.ts`:
- Line 57: trailContextRef is only set once via useRef(initialTrailContext) and
can become stale when initialTrailContext changes, causing sendMessage to read
the wrong context; add a useEffect that watches initialTrailContext and assigns
trailContextRef.current = initialTrailContext whenever it changes. Update the
hook where trailContextRef is declared (trailContextRef, initialTrailContext)
and ensure this effect runs after mount so sendMessage (and any callers reading
trailContextRef.current) always see the latest TrailQAContext.
- Around line 60-67: The initialize() function can be called concurrently and
overwrites providerRef.current without disposing the previous provider; add a
re-entry guard (e.g., an isInitializing boolean ref or check) to prevent
concurrent runs, and before assigning providerRef.current = provider call and
await dispose() on any existing providerRef.current (and handle/nullify it) to
avoid leaks; also update the cleanup path that calls dispose() (e.g., in the
effect cleanup) to await the returned Promise (or ensure dispose is awaited via
an async cleanup wrapper) so provider disposal is fully completed; refer to
initialize, providerRef.current, and dispose to locate the changes.
In `@apps/expo/features/offline-ai/README.md`:
- Around line 19-31: The fenced directory-tree block in README.md is missing a
language identifier which triggers markdownlint MD040; update the fenced code
block in apps/expo/features/offline-ai/README.md by adding a language tag (e.g.,
"text") right after the opening triple backticks for the directory tree so the
block reads ```text and the lint error is resolved.
- Around line 125-130: The example shows two conflicting declarations of the
same variable name; remove the duplicate by showing either the factory usage or
the direct instantiation (not both with the same identifier) — for example, keep
one snippet using createLLMProvider('mock') or rename one variable (e.g.,
mockProvider) when demonstrating new MockLLMProvider(); update the example
around createLLMProvider and MockLLMProvider so only one const provider
declaration exists and initialization (initialize) is shown for the chosen
variable.
---
Nitpick comments:
In `@apps/expo/features/offline-ai/screens/OfflineAIScreen.tsx`:
- Around line 13-32: The screen currently hardcodes DEMO_TRAIL inside
OfflineAIScreen which prevents passing real trail data; update OfflineAIScreen
to accept an external trail (via props or navigation/route params) and pass that
into OfflineAIChat, using DEMO_TRAIL only as a fallback when no trail is
provided; locate DEMO_TRAIL, the OfflineAIScreen function and the OfflineAIChat
usage to implement the prop or route param read and fallback logic so the screen
can be reused for non-demo flows.
In `@vitest.config.ts`:
- Line 6: Update the vitest include globs so they also match .test.tsx files:
modify the include array (the include property in vitest.config.ts) to add
patterns that cover .test.tsx (e.g., expand '**/*.test.ts' and
'**/__tests__/**/*.test.ts' to include tsx or use a {ts,tsx} variant) so future
component tests under apps/expo/components/ will be discovered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c205b30-817d-43e4-b8d2-9a14b82bd0aa
📒 Files selected for processing (9)
apps/expo/features/offline-ai/README.mdapps/expo/features/offline-ai/__tests__/offline-ai.test.tsapps/expo/features/offline-ai/components/OfflineAIChat.tsxapps/expo/features/offline-ai/hooks/useOfflineAI.tsapps/expo/features/offline-ai/index.tsapps/expo/features/offline-ai/screens/OfflineAIScreen.tsxapps/expo/features/offline-ai/types.tspackages/api/package.jsonvitest.config.ts
💤 Files with no reviewable changes (1)
- packages/api/package.json
| const styles = StyleSheet.create({ | ||
| container: { | ||
| flex: 1, | ||
| backgroundColor: '#F9FAFB', | ||
| }, | ||
| header: { | ||
| padding: 16, | ||
| backgroundColor: '#FFFFFF', | ||
| borderBottomWidth: 1, | ||
| borderBottomColor: '#E5E7EB', | ||
| alignItems: 'center', | ||
| }, | ||
| title: { | ||
| fontSize: 20, | ||
| fontWeight: '700', | ||
| color: '#111827', | ||
| }, | ||
| modelInfo: { | ||
| fontSize: 12, | ||
| color: '#6B7280', | ||
| marginTop: 4, | ||
| }, | ||
| initButton: { | ||
| marginTop: 8, | ||
| paddingHorizontal: 16, | ||
| paddingVertical: 8, | ||
| backgroundColor: '#10B981', | ||
| borderRadius: 20, | ||
| }, | ||
| initButtonText: { | ||
| color: '#FFFFFF', | ||
| fontWeight: '600', | ||
| }, | ||
| trailBadge: { | ||
| backgroundColor: '#ECFDF5', | ||
| paddingVertical: 6, | ||
| paddingHorizontal: 12, | ||
| alignItems: 'center', | ||
| }, | ||
| trailBadgeText: { | ||
| color: '#059669', | ||
| fontSize: 14, | ||
| fontWeight: '500', | ||
| }, | ||
| errorContainer: { | ||
| backgroundColor: '#FEF2F2', | ||
| padding: 12, | ||
| marginHorizontal: 16, | ||
| marginTop: 8, | ||
| borderRadius: 8, | ||
| }, | ||
| errorText: { | ||
| color: '#DC2626', | ||
| fontSize: 14, | ||
| }, | ||
| messagesContainer: { | ||
| flex: 1, | ||
| }, | ||
| messagesContent: { | ||
| padding: 16, | ||
| paddingBottom: 24, | ||
| }, | ||
| emptyState: { | ||
| alignItems: 'center', | ||
| paddingVertical: 40, | ||
| }, | ||
| emptyTitle: { | ||
| fontSize: 18, | ||
| fontWeight: '600', | ||
| color: '#111827', | ||
| marginBottom: 8, | ||
| }, | ||
| emptySubtitle: { | ||
| fontSize: 14, | ||
| color: '#6B7280', | ||
| textAlign: 'center', | ||
| paddingHorizontal: 32, | ||
| lineHeight: 20, | ||
| }, | ||
| quickPrompts: { | ||
| marginTop: 24, | ||
| flexDirection: 'row', | ||
| flexWrap: 'wrap', | ||
| justifyContent: 'center', | ||
| gap: 8, | ||
| }, | ||
| quickPromptButton: { | ||
| backgroundColor: '#E0F2FE', | ||
| paddingHorizontal: 16, | ||
| paddingVertical: 10, | ||
| borderRadius: 20, | ||
| }, | ||
| quickPromptText: { | ||
| color: '#0369A1', | ||
| fontSize: 14, | ||
| fontWeight: '500', | ||
| }, | ||
| messageBubble: { | ||
| maxWidth: '80%', | ||
| padding: 12, | ||
| borderRadius: 16, | ||
| marginBottom: 12, | ||
| }, | ||
| userBubble: { | ||
| alignSelf: 'flex-end', | ||
| backgroundColor: '#10B981', | ||
| }, | ||
| assistantBubble: { | ||
| alignSelf: 'flex-start', | ||
| backgroundColor: '#FFFFFF', | ||
| borderWidth: 1, | ||
| borderColor: '#E5E7EB', | ||
| }, | ||
| messageText: { | ||
| fontSize: 15, | ||
| lineHeight: 22, | ||
| }, | ||
| userText: { | ||
| color: '#FFFFFF', | ||
| }, | ||
| assistantText: { | ||
| color: '#111827', | ||
| }, | ||
| timestamp: { | ||
| fontSize: 10, | ||
| color: '#9CA3AF', | ||
| marginTop: 4, | ||
| alignSelf: 'flex-end', | ||
| }, | ||
| typingIndicator: { | ||
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
| padding: 12, | ||
| }, | ||
| typingText: { | ||
| marginLeft: 8, | ||
| color: '#6B7280', | ||
| fontSize: 14, | ||
| }, | ||
| inputContainer: { | ||
| flexDirection: 'row', | ||
| padding: 12, | ||
| backgroundColor: '#FFFFFF', | ||
| borderTopWidth: 1, | ||
| borderTopColor: '#E5E7EB', | ||
| alignItems: 'flex-end', | ||
| }, | ||
| input: { | ||
| flex: 1, | ||
| backgroundColor: '#F3F4F6', | ||
| borderRadius: 20, | ||
| paddingHorizontal: 16, | ||
| paddingVertical: 10, | ||
| fontSize: 15, | ||
| maxHeight: 100, | ||
| color: '#111827', | ||
| }, | ||
| sendButton: { | ||
| marginLeft: 12, | ||
| backgroundColor: '#10B981', | ||
| paddingHorizontal: 20, | ||
| paddingVertical: 10, | ||
| borderRadius: 20, | ||
| }, | ||
| sendButtonDisabled: { | ||
| backgroundColor: '#9CA3AF', | ||
| }, | ||
| sendButtonText: { | ||
| color: '#FFFFFF', | ||
| fontWeight: '600', | ||
| fontSize: 15, | ||
| }, | ||
| clearButton: { | ||
| alignSelf: 'center', | ||
| padding: 8, | ||
| marginBottom: 8, | ||
| }, | ||
| clearButtonText: { | ||
| color: '#6B7280', | ||
| fontSize: 14, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare styling patterns in the offline-ai feature files.
rg -n -C1 'StyleSheet\.create\(|className=' \
apps/expo/features/offline-ai/components/OfflineAIChat.tsx \
apps/expo/features/offline-ai/screens/OfflineAIScreen.tsxRepository: PackRat-AI/PackRat
Length of output: 538
🏁 Script executed:
#!/bin/bash
# Check for NativeWind or Tailwind configuration in the project
echo "=== Checking for NativeWind/Tailwind config ==="
find . -maxdepth 2 -type f \( -name "nativewind.config.*" -o -name "tailwind.config.*" -o -name "metro.config.*" \) | head -20
echo -e "\n=== Checking package.json for styling dependencies ==="
cat package.json | grep -A 5 -B 5 "nativewind\|tailwind"Repository: PackRat-AI/PackRat
Length of output: 165
🏁 Script executed:
#!/bin/bash
# Check styling patterns across other expo components
echo "=== Other expo components styling patterns ==="
find apps/expo/components -name "*.tsx" -type f | head -10 | while read file; do
echo "File: $file"
grep -n "StyleSheet\|className=" "$file" | head -3
echo "---"
doneRepository: PackRat-AI/PackRat
Length of output: 1799
🏁 Script executed:
#!/bin/bash
# Examine a modern component using className to confirm the pattern
echo "=== Card.tsx (modern styling) ==="
head -15 apps/expo/components/Card.tsx
echo -e "\n=== OfflineAIScreen.tsx styling check ==="
sed -n '35,60p' apps/expo/features/offline-ai/screens/OfflineAIScreen.tsxRepository: PackRat-AI/PackRat
Length of output: 765
🏁 Script executed:
#!/bin/bash
# Check if the package.json includes nativewind at the top level or in any workspace
echo "=== Full package.json dependencies section ==="
cat package.json | jq '.dependencies, .devDependencies' 2>/dev/null | grep -i "native\|wind\|tailwind" -B2 -A2
echo -e "\n=== Checking for nativewind in monorepo structure ==="
find . -maxdepth 3 -name "package.json" -type f -exec grep -l "nativewind\|tailwind" {} \;Repository: PackRat-AI/PackRat
Length of output: 270
🏁 Script executed:
#!/bin/bash
# Verify the UI package structure to confirm className support
echo "=== `@packrat/ui` exports ==="
head -50 packages/ui/nativewindui/index.ts 2>/dev/null || head -50 packages/ui/nativewind/index.ts 2>/dev/null || find packages/ui -name "*.ts" -type f | head -1 | xargs head -50
echo -e "\n=== OfflineAIChat.tsx line 201-210 ==="
sed -n '201,210p' apps/expo/features/offline-ai/components/OfflineAIChat.tsxRepository: PackRat-AI/PackRat
Length of output: 392
Refactor to use NativeWind className styling instead of StyleSheet.create.
This component violates the project's styling guideline: "Use Tailwind CSS with custom UI components for styling across applications." Modern components in the codebase (Card, CategoriesFilter, ErrorState) use className with Tailwind classes via @packrat/ui/nativewindui. Replace the StyleSheet.create block with className attributes using Tailwind equivalents (e.g., flex-1 bg-gray-50 instead of {flex: 1, backgroundColor: '#F9FAFB'}).
Suggested direction
-import {
- ActivityIndicator,
- KeyboardAvoidingView,
- Platform,
- ScrollView,
- StyleSheet,
- Text,
- TextInput,
- TouchableOpacity,
- View,
-} from 'react-native';
+import {
+ ActivityIndicator,
+ KeyboardAvoidingView,
+ Platform,
+ ScrollView,
+ Text,
+ TextInput,
+ TouchableOpacity,
+ View,
+} from 'react-native';
@@
- <KeyboardAvoidingView
- style={styles.container}
+ <KeyboardAvoidingView
+ className="flex-1 bg-gray-50"
behavior={Platform.OS === 'ios' ? 'padding' : 'height'}
keyboardVerticalOffset={100}
>
@@
- <View style={styles.header}>
- <Text style={styles.title}>🏔️ Offline AI Assistant</Text>
+ <View className="items-center border-b border-gray-200 bg-white p-4">
+ <Text className="text-xl font-bold text-gray-900">🏔️ Offline AI Assistant</Text>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/offline-ai/components/OfflineAIChat.tsx` around lines 201
- 382, The file defines a large styles object (styles created via
StyleSheet.create) and uses it throughout OfflineAIChat; replace that with
NativeWind/Tailwind className strings: remove the styles constant and any
StyleSheet import, convert each usage of styles.container, styles.header,
styles.title, modelInfo, initButton, trailBadge, errorContainer,
messagesContainer, messagesContent, emptyState, quickPrompts, quickPromptButton,
messageBubble, userBubble, assistantBubble, messageText, userText,
assistantText, timestamp, typingIndicator, inputContainer, input, sendButton,
sendButtonDisabled, clearButton, etc., into equivalent Tailwind className values
(e.g., styles.container -> "flex-1 bg-gray-50", header -> "p-4 bg-white border-b
border-gray-200 items-center", title -> "text-lg font-extrabold text-gray-900",
etc.) and update the JSX elements (View, Text, ScrollView, TextInput,
TouchableOpacity) to use the className prop provided by `@packrat/ui/nativewindui`
(or nativewind) instead of style={styles.xxx}; ensure conditional styles like
sendButtonDisabled are applied by composing className strings conditionally;
keep all existing component/prop names (e.g., OfflineAIChat, any handlers)
unchanged.
| const [modelInfo, setModelInfo] = useState<{ name: string; size: string } | null>(null); | ||
|
|
||
| const providerRef = useRef<LLMProvider | null>(null); | ||
| const trailContextRef = useRef<TrailQAContext | undefined>(initialTrailContext); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether initialTrailContext is kept in sync after initial render.
rg -n -C2 'initialTrailContext|trailContextRef = useRef|setTrailContext|useEffect' apps/expo/features/offline-ai/hooks/useOfflineAI.tsRepository: PackRat-AI/PackRat
Length of output: 1418
🏁 Script executed:
cat -n apps/expo/features/offline-ai/hooks/useOfflineAI.tsRepository: PackRat-AI/PackRat
Length of output: 6479
Add a useEffect to synchronize trailContextRef when initialTrailContext changes.
trailContextRef is initialized once on mount and never updated when the trailContext option changes. This causes sendMessage (line 119) to use a stale context value. A useEffect that updates trailContextRef.current whenever initialTrailContext changes is required.
Suggested change
@@
interface UseOfflineAIReturn extends OfflineAIState {
@@
- setTrailContext: (context: TrailQAContext) => void;
+ setTrailContext: (context?: TrailQAContext) => void;
@@
- const setTrailContext = useCallback((context: TrailQAContext) => {
+ const setTrailContext = useCallback((context?: TrailQAContext) => {
trailContextRef.current = context;
}, []);
+
+ useEffect(() => {
+ trailContextRef.current = initialTrailContext;
+ }, [initialTrailContext]);Also applies to: 93-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/offline-ai/hooks/useOfflineAI.ts` at line 57,
trailContextRef is only set once via useRef(initialTrailContext) and can become
stale when initialTrailContext changes, causing sendMessage to read the wrong
context; add a useEffect that watches initialTrailContext and assigns
trailContextRef.current = initialTrailContext whenever it changes. Update the
hook where trailContextRef is declared (trailContextRef, initialTrailContext)
and ensure this effect runs after mount so sendMessage (and any callers reading
trailContextRef.current) always see the latest TrailQAContext.
| const initialize = useCallback(async () => { | ||
| try { | ||
| setError(null); | ||
|
|
||
| // Create provider | ||
| const provider = createLLMProvider('mock'); | ||
| providerRef.current = provider; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify assignment/disposal paths in useOfflineAI lifecycle.
rg -n -C3 'initialize = useCallback|providerRef\.current = provider|providerRef\.current\?\.dispose\(' apps/expo/features/offline-ai/hooks/useOfflineAI.tsRepository: PackRat-AI/PackRat
Length of output: 605
🏁 Script executed:
cat -n apps/expo/features/offline-ai/hooks/useOfflineAI.ts | head -100Repository: PackRat-AI/PackRat
Length of output: 3813
🏁 Script executed:
rg -n 'initialize\(' apps/expo/features/offline-ai/hooks/useOfflineAI.tsRepository: PackRat-AI/PackRat
Length of output: 131
🏁 Script executed:
rg -n 'trailContextRef' apps/expo/features/offline-ai/hooks/useOfflineAI.tsRepository: PackRat-AI/PackRat
Length of output: 242
🏁 Script executed:
sed -n '145,165p' apps/expo/features/offline-ai/hooks/useOfflineAI.tsRepository: PackRat-AI/PackRat
Length of output: 517
🏁 Script executed:
rg -n 'interface LLMProvider|type LLMProvider' apps/expo/features/offline-ai/Repository: PackRat-AI/PackRat
Length of output: 134
🏁 Script executed:
sed -n '69,120p' apps/expo/features/offline-ai/types.tsRepository: PackRat-AI/PackRat
Length of output: 1828
🏁 Script executed:
rg -n '\.initialize\(|initialize\(\)' apps/expo/features/offline-ai/Repository: PackRat-AI/PackRat
Length of output: 1303
🏁 Script executed:
rg -n 'useOfflineAI' apps/expo/features/offline-ai/Repository: PackRat-AI/PackRat
Length of output: 848
🏁 Script executed:
grep -n 'initialize' apps/expo/features/offline-ai/components/OfflineAIChat.tsxRepository: PackRat-AI/PackRat
Length of output: 159
Guard initialize() re-entry and dispose replaced providers.
The initialize() function is exposed and used as a button handler (OfflineAIChat.tsx:90), allowing repeated or concurrent calls. Each call overwrites providerRef.current without disposing the previous provider, risking resource leaks. Additionally, the cleanup function at line 157 calls dispose() without awaiting the returned Promise.
Suggested change
@@
const providerRef = useRef<LLMProvider | null>(null);
+ const initializingRef = useRef(false);
@@
const initialize = useCallback(async () => {
+ if (initializingRef.current) return;
+ initializingRef.current = true;
try {
setError(null);
+ setIsAvailable(false);
+
+ if (providerRef.current) {
+ await providerRef.current.dispose();
+ }
// Create provider
const provider = createLLMProvider('mock');
providerRef.current = provider;
@@
setIsAvailable(true);
onReady?.();
} catch (err) {
const errorMessage = err instanceof Error ? err.message : 'Failed to initialize AI';
setError(errorMessage);
+ setIsAvailable(false);
onError?.(errorMessage);
+ } finally {
+ initializingRef.current = false;
}
}, [config, onReady, onError]);
@@
return () => {
- providerRef.current?.dispose();
+ void providerRef.current?.dispose().catch(() => {
+ // swallow during unmount
+ });
};
}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/offline-ai/hooks/useOfflineAI.ts` around lines 60 - 67,
The initialize() function can be called concurrently and overwrites
providerRef.current without disposing the previous provider; add a re-entry
guard (e.g., an isInitializing boolean ref or check) to prevent concurrent runs,
and before assigning providerRef.current = provider call and await dispose() on
any existing providerRef.current (and handle/nullify it) to avoid leaks; also
update the cleanup path that calls dispose() (e.g., in the effect cleanup) to
await the returned Promise (or ensure dispose is awaited via an async cleanup
wrapper) so provider disposal is fully completed; refer to initialize,
providerRef.current, and dispose to locate the changes.
| ``` | ||
| features/offline-ai/ | ||
| ├── components/ | ||
| │ └── OfflineAIChat.tsx # Main chat UI component | ||
| ├── hooks/ | ||
| │ └── useOfflineAI.ts # Main hook for AI functionality | ||
| ├── lib/ | ||
| │ └── mock-llm-provider.ts # Mock LLM provider (dev/testing) | ||
| ├── screens/ | ||
| │ └── OfflineAIScreen.tsx # Full screen view | ||
| ├── types.ts # TypeScript type definitions | ||
| └── index.ts # Public exports | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the directory-tree fenced block.
This fence is missing a language tag, which triggers markdownlint (MD040).
Suggested change
-```
+```text
features/offline-ai/
├── components/
│ └── OfflineAIChat.tsx # Main chat UI component
├── hooks/
│ └── useOfflineAI.ts # Main hook for AI functionality
├── lib/
│ └── mock-llm-provider.ts # Mock LLM provider (dev/testing)
├── screens/
│ └── OfflineAIScreen.tsx # Full screen view
├── types.ts # TypeScript type definitions
└── index.ts # Public exports</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @apps/expo/features/offline-ai/README.md around lines 19 - 31, The fenced
directory-tree block in README.md is missing a language identifier which
triggers markdownlint MD040; update the fenced code block in
apps/expo/features/offline-ai/README.md by adding a language tag (e.g., "text")
right after the opening triple backticks for the directory tree so the block
reads ```text and the lint error is resolved.
</details>
<!-- fingerprinting:phantom:medusa:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
| // Using factory | ||
| const provider = createLLMProvider('mock'); | ||
|
|
||
| // Direct usage | ||
| const provider = new MockLLMProvider(); | ||
| await provider.initialize({ |
There was a problem hiding this comment.
Fix duplicate provider declaration in the same example snippet.
The snippet redeclares const provider, so it can’t be copied as-is.
Suggested change
// Using factory
-const provider = createLLMProvider('mock');
+const factoryProvider = createLLMProvider('mock');
// Direct usage
-const provider = new MockLLMProvider();
-await provider.initialize({
+const directProvider = new MockLLMProvider();
+await directProvider.initialize({
modelId: 'llama-3.2-1b',
modelPath: './models/llama-3.2-1b-q4_k_m.gguf',
maxTokens: 512,
temperature: 0.7,
contextWindow: 2048,
gpuLayers: 0,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/offline-ai/README.md` around lines 125 - 130, The example
shows two conflicting declarations of the same variable name; remove the
duplicate by showing either the factory usage or the direct instantiation (not
both with the same identifier) — for example, keep one snippet using
createLLMProvider('mock') or rename one variable (e.g., mockProvider) when
demonstrating new MockLLMProvider(); update the example around createLLMProvider
and MockLLMProvider so only one const provider declaration exists and
initialization (initialize) is shown for the chosen variable.
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 `@apps/expo/features/offline-ai/components/OfflineAIChat.tsx`:
- Around line 90-92: In OfflineAIChat, add accessibilityLabel and
accessibilityRole="button" to each TouchableOpacity so screen readers can
identify them; specifically update the TouchableOpacity using the initialize
handler to include accessibilityLabel="Enable offline AI" and
accessibilityRole="button", and similarly add descriptive accessibilityLabel
values for the other touchables (e.g., the send message button, clear history
button, and stop generation button) where handlers like sendMessage,
clearConversation, and stopGeneration are used so each interactive element has a
unique, human-friendly label.
- Line 48: In the OfflineAIChat component remove the unused destructured prop
setTrailContext from the props/object destructuring (it's not referenced
anywhere in OfflineAIChat and trailContext is already provided via the
trailContext option during initialization), so update the destructuring to drop
setTrailContext and ensure no other references to setTrailContext remain in
OfflineAIChat.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f0dafaa-93c1-4a64-a873-5761346eb182
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/expo/features/guides/components/GuideCard.tsxapps/expo/features/offline-ai/__tests__/offline-ai.test.tsapps/expo/features/offline-ai/components/OfflineAIChat.tsxapps/expo/features/offline-ai/screens/OfflineAIScreen.tsx
💤 Files with no reviewable changes (1)
- apps/expo/features/guides/components/GuideCard.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/expo/features/offline-ai/tests/offline-ai.test.ts
- apps/expo/features/offline-ai/screens/OfflineAIScreen.tsx
| <TouchableOpacity onPress={initialize} style={styles.initButton}> | ||
| <Text style={styles.initButtonText}>Tap to Enable</Text> | ||
| </TouchableOpacity> |
There was a problem hiding this comment.
Add accessibility labels to interactive elements.
All TouchableOpacity buttons lack accessibilityLabel and accessibilityRole props, making the chat interface difficult or impossible to use with screen readers.
♿ Proposed accessibility improvements
- <TouchableOpacity onPress={initialize} style={styles.initButton}>
+ <TouchableOpacity
+ onPress={initialize}
+ style={styles.initButton}
+ accessibilityRole="button"
+ accessibilityLabel="Enable offline AI assistant"
+ > <TouchableOpacity
key={prompt}
style={styles.quickPromptButton}
onPress={() => sendMessage(prompt)}
disabled={!isAvailable || isGenerating}
+ accessibilityRole="button"
+ accessibilityLabel={`Ask: ${prompt}`}
> <TouchableOpacity
style={[
styles.sendButton,
(!input.trim() || !isAvailable || isGenerating) && styles.sendButtonDisabled,
]}
onPress={handleSend}
disabled={!input.trim() || !isAvailable || isGenerating}
+ accessibilityRole="button"
+ accessibilityLabel="Send message"
>- <TouchableOpacity style={styles.clearButton} onPress={clearMessages}>
+ <TouchableOpacity
+ style={styles.clearButton}
+ onPress={clearMessages}
+ accessibilityRole="button"
+ accessibilityLabel="Clear chat history"
+ >Also applies to: 126-133, 161-170, 175-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/offline-ai/components/OfflineAIChat.tsx` around lines 90 -
92, In OfflineAIChat, add accessibilityLabel and accessibilityRole="button" to
each TouchableOpacity so screen readers can identify them; specifically update
the TouchableOpacity using the initialize handler to include
accessibilityLabel="Enable offline AI" and accessibilityRole="button", and
similarly add descriptive accessibilityLabel values for the other touchables
(e.g., the send message button, clear history button, and stop generation
button) where handlers like sendMessage, clearConversation, and stopGeneration
are used so each interactive element has a unique, human-friendly label.
Deploying packrat-guides with
|
| Latest commit: |
443b00d
|
| Status: | 🚫 Build failed. |
Deploying packrat-landing with
|
| Latest commit: |
0dd7e3c
|
| Status: | 🚫 Build failed. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/api/test/setup.ts (1)
424-425: Acceptable type workaround for test setup.The
as anycast with the biome-ignore comment is a reasonable approach for test setup code. The type mismatch occurs becauseReturnType<typeof drizzle>(line 53) captures the base function signature, whiledrizzle(testClient, { schema })returns a schema-aware database type.If you want to eliminate the cast in the future, consider using the proper DrizzleORM type:
♻️ Optional: Use explicit DrizzleORM type
-import { drizzle } from 'drizzle-orm/node-postgres'; +import { drizzle, type NodePgDatabase } from 'drizzle-orm/node-postgres';Then update line 53:
-let testDb: ReturnType<typeof drizzle>; +let testDb: NodePgDatabase<typeof schema>;And remove the cast on line 425:
- // biome-ignore lint/suspicious/noExplicitAny: Test setup needs any for drizzle client - testDb = drizzle(testClient, { schema }) as any; + testDb = drizzle(testClient, { schema });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/test/setup.ts` around lines 424 - 425, The current test setup uses an acceptable workaround by casting drizzle(testClient, { schema }) to any (testDb) with a biome-ignore; to remove this cast later, update the shared type alias that currently uses ReturnType<typeof drizzle> (the declaration referenced near the top of the file) to a schema-aware Drizzle ORM database type compatible with your schema, then remove the biome-ignore and the "as any" on the drizzle(testClient, { schema }) assignment (affecting testDb, drizzle, testClient, and schema references). Ensure the new type aligns with the schema-aware return type from drizzle so the explicit cast is no longer needed.apps/expo/lib/i18n/EXAMPLES.tsx (2)
59-60: Prefer a tiny example type over suppressingany.This helper only depends on
anyis broader than needed. A narrow input shape keeps the example type-safe and removes the ignore.Proposed diff
-// biome-ignore lint/suspicious/noExplicitAny: Example code uses any for flexibility -export function validateForm(formData: any) { +type ExampleFormData = { + email?: string; +}; + +export function validateForm(formData: ExampleFormData) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/lib/i18n/EXAMPLES.tsx` around lines 59 - 60, The validateForm function currently accepts formData: any; tighten the example by replacing the any with a small explicit shape that matches what the helper uses (e.g., an interface or inline type with an email property such as { email?: string } or { email: string }), remove the biome-ignore comment (lint/suspicious/noExplicitAny), and update any internal references in validateForm to use that typed parameter (function validateForm(formData: { email?: string })) so the example is type-safe without suppressing the lint.
30-31: Drop the unused prop instead of suppressing the lint.
InterpolationExampleexports auserNameprop but never demonstrates it, which makes the sample API misleading. If the example is meant to stay date-based, removing the prop is cleaner than carrying a permanent ignore.Proposed diff
-// biome-ignore lint/correctness/noUnusedFunctionParameters: Intentionally unused - example code -export function InterpolationExample({ userName }: { userName: string }) { +export function InterpolationExample() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/lib/i18n/EXAMPLES.tsx` around lines 30 - 31, Remove the unused userName prop from the InterpolationExample export instead of suppressing the lint; delete the biome-ignore comment and change the function signature InterpolationExample({ userName }: { userName: string }) to a no-arg export function InterpolationExample() (and remove any related type annotation), and update any local references or sample call sites to no longer pass userName so the example API accurately reflects the demonstrated usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/expo/lib/i18n/EXAMPLES.tsx`:
- Around line 59-60: The validateForm function currently accepts formData: any;
tighten the example by replacing the any with a small explicit shape that
matches what the helper uses (e.g., an interface or inline type with an email
property such as { email?: string } or { email: string }), remove the
biome-ignore comment (lint/suspicious/noExplicitAny), and update any internal
references in validateForm to use that typed parameter (function
validateForm(formData: { email?: string })) so the example is type-safe without
suppressing the lint.
- Around line 30-31: Remove the unused userName prop from the
InterpolationExample export instead of suppressing the lint; delete the
biome-ignore comment and change the function signature InterpolationExample({
userName }: { userName: string }) to a no-arg export function
InterpolationExample() (and remove any related type annotation), and update any
local references or sample call sites to no longer pass userName so the example
API accurately reflects the demonstrated usage.
In `@packages/api/test/setup.ts`:
- Around line 424-425: The current test setup uses an acceptable workaround by
casting drizzle(testClient, { schema }) to any (testDb) with a biome-ignore; to
remove this cast later, update the shared type alias that currently uses
ReturnType<typeof drizzle> (the declaration referenced near the top of the file)
to a schema-aware Drizzle ORM database type compatible with your schema, then
remove the biome-ignore and the "as any" on the drizzle(testClient, { schema })
assignment (affecting testDb, drizzle, testClient, and schema references).
Ensure the new type aligns with the schema-aware return type from drizzle so the
explicit cast is no longer needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9259e10-8bd5-4cbd-b2c3-a4076c272441
📒 Files selected for processing (2)
apps/expo/lib/i18n/EXAMPLES.tsxpackages/api/test/setup.ts
|
👋 @copilot |
|
@copilot |
3 similar comments
|
@copilot |
|
@copilot |
|
@copilot |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 30 changed files in this pull request and generated 9 comments.
You can also share your feedback on Copilot code review. Take the survey.
| async initialize(config: LLMConfig): Promise<void> { | ||
| // Simulate model loading delay | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
|
|
There was a problem hiding this comment.
MockLLMProvider adds real setTimeout delays, which slows unit tests and can make the suite unnecessarily long/flaky. Consider making delays optional (constructor/config/env flag) or using fake timers in tests so behavior stays deterministic without real waiting.
|
|
||
| <div className="mt-2 flex flex-wrap gap-2"> | ||
| {Array.from({ length: 3 }).map((_, i) => ( | ||
| {integration.features?.map((feature, i) => ( |
There was a problem hiding this comment.
The map callback includes the i parameter but it’s unused. With Biome’s recommended rules this may trigger an unused-variable lint error; consider removing the parameter or renaming to _i if you want to keep it for readability.
| {integration.features?.map((feature, i) => ( | |
| {integration.features?.map((feature) => ( |
| > | ||
| <div className="flex -space-x-2"> | ||
| {[1, 2, 3, 4].map((i) => ( | ||
| {['Hiker', 'Camper', 'Climber', 'Explorer'].map((role, i) => ( |
There was a problem hiding this comment.
The map callback takes (role, i) but i is no longer used after switching the key to role. Consider removing i (or renaming to _i) to avoid unused-variable lint warnings.
| {['Hiker', 'Camper', 'Climber', 'Explorer'].map((role, i) => ( | |
| {['Hiker', 'Camper', 'Climber', 'Explorer'].map((role) => ( |
| include: ['**/__tests__/**/*.test.ts', '**/*.test.ts'], | ||
| exclude: ['node_modules', 'dist', '.expo', 'ios', 'android'], |
There was a problem hiding this comment.
This root Vitest config will match all "**/*.test.ts" files in the monorepo (including packages/api tests). Those API tests rely on packages/api/vitest.config.ts (pool workers + globalSetup + setupFiles), so running Vitest from the repo root would execute them without the required setup and likely fail. Consider narrowing include/exclude to just the intended test area (e.g., apps/expo/features/offline-ai) or configuring a Vitest workspace/projects setup so each package uses its own config.
| include: ['**/__tests__/**/*.test.ts', '**/*.test.ts'], | |
| exclude: ['node_modules', 'dist', '.expo', 'ios', 'android'], | |
| include: [ | |
| 'apps/expo/features/offline-ai/**/__tests__/**/*.test.ts', | |
| 'apps/expo/features/offline-ai/**/*.test.ts', | |
| ], | |
| exclude: ['node_modules', 'dist', '.expo', 'ios', 'android', 'packages/api'], |
| if (isCI) { | ||
| // In CI, bunfig.toml will use PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN | ||
| // In CI, the env var is set before install so bunfig.toml can read it directly | ||
| if (!process.env.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN) { | ||
| console.error('❌ PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN not found in CI'); | ||
| console.error('PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN not found in CI'); | ||
| console.error('Please ensure PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN is set in your CI secrets'); |
There was a problem hiding this comment.
The CI branch comment says bunfig.toml can read the token from the env var directly, but this script now writes a .npmrc and bunfig.toml no longer has a token field. Please update the comment/header docs to reflect that auth is provided via the generated .npmrc in CI/local flows.
|
|
||
| // Cleanup on unmount | ||
| return () => { | ||
| providerRef.current?.dispose(); |
There was a problem hiding this comment.
The effect cleanup calls dispose() (async) without awaiting/handling the returned Promise. This can trip Biome’s no-floating-promises rule and can surface as an unhandled rejection if dispose ever throws. Consider explicitly discarding/handling it (e.g., void ...dispose() with internal try/catch if needed).
| providerRef.current?.dispose(); | |
| const provider = providerRef.current; | |
| if (provider) { | |
| void provider.dispose().catch(() => { | |
| // Swallow errors on cleanup to avoid unhandled rejections | |
| }); | |
| } |
| bun test | ||
|
|
||
| # Or with coverage | ||
| bun test:coverage |
There was a problem hiding this comment.
The README suggests running bun test / bun test:coverage, but these tests are written for Vitest (import ... from 'vitest') and there’s no root test script. Please update the commands to the expected invocation (e.g., bunx vitest run), or add the corresponding scripts.
| bun test | |
| # Or with coverage | |
| bun test:coverage | |
| cd apps/expo | |
| # Run tests for the Offline AI feature | |
| bunx vitest run features/offline-ai | |
| # Or with coverage | |
| bunx vitest run features/offline-ai --coverage |
| uri: imageUrl, | ||
| ...(Platform.OS === 'android' | ||
| ? { | ||
| headers: { | ||
| 'User-Agent': | ||
| 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/135.0.0.0 Safari/537.36', | ||
| Accept: 'image/webp,image/apng,image/*,*/*;q=0.8', | ||
| }, | ||
| } | ||
| : {}), | ||
| headers: { | ||
| 'User-Agent': | ||
| 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/135.0.0.0 Safari/537.36', | ||
| Accept: 'image/webp,image/apng,image/*,*/*;q=0.8', |
There was a problem hiding this comment.
Since headers are now unconditional, the Platform import at the top of the file is unused (it was only needed for the Android-only branch). Please remove the unused Platform import to avoid lint/TS unused-import failures.
|
|
||
| const providerRef = useRef<LLMProvider | null>(null); | ||
| const trailContextRef = useRef<TrailQAContext | undefined>(initialTrailContext); | ||
|
|
There was a problem hiding this comment.
trailContext is only captured on first render (useRef(initialTrailContext)) and won’t update if the caller passes a new trailContext later. Consider syncing the ref in a useEffect when initialTrailContext changes (or documenting that callers must use setTrailContext).
| // Keep trail context ref in sync with options.trailContext when it changes | |
| useEffect(() => { | |
| trailContextRef.current = initialTrailContext; | |
| }, [initialTrailContext]); |
|
👋 @copilot |
|
@andrew-bierman I've opened a new pull request, #1873, to work on those changes. Once the pull request is ready, I'll request review from you. |
andrew-bierman
changed the title
Add 14 new Maestro E2E flows plus a standalone suite runner script,
extending the existing .maestro/ suite on development with broader
coverage across AI chat, catalog search, pack management, trip editing,
dashboard templates, guides, session persistence, and negative paths.
New flows:
- ai/ai-chat-dashboard-flow.yaml
- ai/ai-chat-pack-flow.yaml
- catalog/catalog-search-flow.yaml
- dashboard/pack-templates-flow.yaml
- guides/guides-browse-flow.yaml
- helpers/handle-chooser.yaml
- negative/invalid-login-flow.yaml
- packs/add-item-catalog-flow.yaml
- packs/add-item-in-pack-catalog-flow.yaml
- packs/add-item-manual-flow.yaml
- packs/pack-edit-share-flow.yaml
- packs/pack-toggle-filter-flow.yaml
- setup/session-persistence-flow.yaml
- trips/trip-edit-flow.yaml
All new flows use the upstream \${APP_ID} env-var convention from #2040
so they work with any bundle identifier. Also adds .maestro/run-suite.sh
which runs the full ordered suite locally with a single command, wiring
through APP_ID, TEST_EMAIL, and TEST_PASSWORD.
Origin: extracted from #1867 (scope-clean split).
|
Opening a scope-clean version at #2048 that pulls only the |
andrew-bierman
force-pushed
the
qa/fix-install-and-tests
branch
from
fe9828d to
e55ffb0
Compare
Add 14 new Maestro E2E flows plus a standalone suite runner script,
extending the existing .maestro/ suite on development with broader
coverage across AI chat, catalog search, pack management, trip editing,
dashboard templates, guides, session persistence, and negative paths.
New flows:
- ai/ai-chat-dashboard-flow.yaml
- ai/ai-chat-pack-flow.yaml
- catalog/catalog-search-flow.yaml
- dashboard/pack-templates-flow.yaml
- guides/guides-browse-flow.yaml
- helpers/handle-chooser.yaml
- negative/invalid-login-flow.yaml
- packs/add-item-catalog-flow.yaml
- packs/add-item-in-pack-catalog-flow.yaml
- packs/add-item-manual-flow.yaml
- packs/pack-edit-share-flow.yaml
- packs/pack-toggle-filter-flow.yaml
- setup/session-persistence-flow.yaml
- trips/trip-edit-flow.yaml
All new flows use the upstream \${APP_ID} env-var convention from #2040
so they work with any bundle identifier. Also adds .maestro/run-suite.sh
which runs the full ordered suite locally with a single command, wiring
through APP_ID, TEST_EMAIL, and TEST_PASSWORD.
Origin: extracted from #1867 (scope-clean split).
Summary
Test Coverage
Hardening Techniques
.*items.*)retryTapIfNoChangefor unreliable element tapscenterElement: trueon scroll for better tap targetingstopApp: truefor clean deep link handlingBugs Documented
Test Plan
🤖 Generated with Claude Code