fix(offline-ai): fix MockLLMProvider to process trail context#1870
Merged
andrew-bierman merged 5 commits intoApr 11, 2026
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a new MockLLMProvider and singleton plus a Vitest test suite that validate generate() behavior across various LLMContext scenarios (trail, activity, weather, optional/missing fields, and systemPrompt), producing deterministic, context-derived string responses. Changes
Sequence Diagram(s)(Skipped — change is a single-provider implementation plus tests; no multi-component sequential flow to visualize.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
apps/expo/features/offline-ai/lib/MockLLMProvider.ts (1)
81-86: Redundant fallback on line 86.The
|| ...fallback is unreachable becauseparts.length === 0is already handled on lines 82-84, soparts.join('')will never be empty at line 86.Proposed simplification
// If we have context but no specific response content, add a default if (parts.length === 0) { return `Hello! How can I help you with your outdoor adventure today?`; } - return parts.join('').trim() || `Hello! How can I help you with your outdoor adventure today?`; + return parts.join('').trim(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/offline-ai/lib/MockLLMProvider.ts` around lines 81 - 86, Redundant fallback: remove the unreachable "|| `Hello! How can I help you with your outdoor adventure today?`" after parts.join('').trim() in MockLLMProvider.ts; keep the existing guard that returns the default when parts.length === 0, and change the final return to simply return parts.join('').trim() (referencing the variable parts and the function/method in MockLLMProvider that builds the response).apps/expo/features/offline-ai/__tests__/offline-ai.test.ts (2)
92-105: Misleading test name: "process multiple trails" only tests one trail.The test description says "multiple trails" but
LLMContext.trailis a singleTrailContext, not an array. The test only provides one trail. Consider renaming for accuracy.Proposed fix
- it('should process multiple trails in context', async () => { + it('should process trail in context', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/offline-ai/__tests__/offline-ai.test.ts` around lines 92 - 105, The test name is misleading: the describe/it titled "should process multiple trails in context" uses a single LLMContext.trail (a TrailContext), not an array; update the test title to reflect a single trail (e.g., rename the it description to "should process a trail in context" or "should include trail from context in response") and leave the body using LLMContext.trail and the expect(...) assertion unchanged so the test accurately describes what generate('Compare trails', { context }) verifies.
66-72: Test passessystemPromptbut doesn't verify its usage.This test confirms the method doesn't throw when
systemPromptis provided, but the implementation ignores it entirely. Either remove this test untilsystemPromptis implemented, or add a TODO comment noting the limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/offline-ai/__tests__/offline-ai.test.ts` around lines 66 - 72, The test "should use system prompt when provided" calls provider.generate('Hello', { systemPrompt: 'You are a helpful hiking assistant.' }) but doesn't verify that systemPrompt is used; update the test to either remove it or mark it as skipped/annotated with a TODO explaining systemPrompt isn't implemented yet, or implement an assertion that verifies provider.generate consumed systemPrompt (e.g., by inspecting the generated payload or mocked provider behavior). Change the test in offline-ai.test.ts (the it block using provider.generate and systemPrompt) accordingly so it no longer gives a false positive.
🤖 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/__tests__/offline-ai.test.ts`:
- Line 1: The import list at the top (import { describe, it, expect, beforeEach
} from 'vitest') is not sorted per Biome rules; run `bun format` to auto-sort
imports or reorder the named imports alphabetically (beforeEach, describe,
expect, it) so the import statement conforms to the project's import-sorting
rules.
- Around line 55-58: The test's regex in the
expect(response.toLowerCase()).toMatch(...) is incorrect: it contains a leading
space before "Jacket" and a capital "J" which will never match after
toLowerCase(), and includes "precipitation" which is unlikely; update the
pattern to use all-lowercase tokens and remove the stray space (e.g., replace
the current pattern with /(rain|wet|rainy|coat|jacket|umbrella)/) in the
expect(response.toLowerCase()).toMatch(...) assertion referencing the response
variable.
In `@apps/expo/features/offline-ai/lib/MockLLMProvider.ts`:
- Around line 21-24: GenerateOptions declares systemPrompt but
MockLLMProvider.generate ignores it; either remove systemPrompt from the
interface or wire it into the generate flow—update the generate method in
MockLLMProvider to accept the options param (GenerateOptions) and incorporate
options.systemPrompt when constructing the LLM response (e.g., prepend or merge
it into the prompt/context handling in generate()), or delete systemPrompt from
the GenerateOptions type and all call sites to keep the API accurate; reference
the GenerateOptions interface and the generate() method in MockLLMProvider when
making the change.
- Around line 39-50: The parts pushed for context.trail in MockLLMProvider (the
code that checks context.trail, trail.name, trail.difficulty, trail.length and
pushes into the parts array) are missing separating spaces which causes
concatenated tokens like "Test Trail(moderate difficulty)which is..."; fix by
including trailing or leading spaces when pushing each segment (e.g., append
"For {trail.name} ", "({trail.difficulty} difficulty) ", and "which is
{trail.length} miles long" or otherwise ensure a single space separator between
parts before joining) so that joins produce properly spaced output.
- Line 27: The generate method on MockLLMProvider currently declares a prompt
parameter that isn't used; either rename the parameter to _prompt to mark it
intentionally unused or explicitly use it in the mock implementation (for
example incorporate prompt into the returned string or a debug log) so the
linter stops reporting it as unused—update the signature in async
generate(prompt: string, options?: GenerateOptions): Promise<string> (or change
to async generate(_prompt: string, options?: GenerateOptions): Promise<string>)
and adjust the method body to reference the parameter if you choose to use it.
---
Nitpick comments:
In `@apps/expo/features/offline-ai/__tests__/offline-ai.test.ts`:
- Around line 92-105: The test name is misleading: the describe/it titled
"should process multiple trails in context" uses a single LLMContext.trail (a
TrailContext), not an array; update the test title to reflect a single trail
(e.g., rename the it description to "should process a trail in context" or
"should include trail from context in response") and leave the body using
LLMContext.trail and the expect(...) assertion unchanged so the test accurately
describes what generate('Compare trails', { context }) verifies.
- Around line 66-72: The test "should use system prompt when provided" calls
provider.generate('Hello', { systemPrompt: 'You are a helpful hiking assistant.'
}) but doesn't verify that systemPrompt is used; update the test to either
remove it or mark it as skipped/annotated with a TODO explaining systemPrompt
isn't implemented yet, or implement an assertion that verifies provider.generate
consumed systemPrompt (e.g., by inspecting the generated payload or mocked
provider behavior). Change the test in offline-ai.test.ts (the it block using
provider.generate and systemPrompt) accordingly so it no longer gives a false
positive.
In `@apps/expo/features/offline-ai/lib/MockLLMProvider.ts`:
- Around line 81-86: Redundant fallback: remove the unreachable "|| `Hello! How
can I help you with your outdoor adventure today?`" after parts.join('').trim()
in MockLLMProvider.ts; keep the existing guard that returns the default when
parts.length === 0, and change the final return to simply return
parts.join('').trim() (referencing the variable parts and the function/method in
MockLLMProvider that builds the response).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 303940e0-669b-457e-bee4-f3150acd5dc0
📒 Files selected for processing (2)
apps/expo/features/offline-ai/__tests__/offline-ai.test.tsapps/expo/features/offline-ai/lib/MockLLMProvider.ts
Comment on lines
+55
to
+58
| expect(response).toBeDefined(); | ||
| // Should mention weather-relevant items | ||
| expect(response.toLowerCase()).toMatch(/rain|wet|precipitation|rainy|coat| Jacket|umbrella/); | ||
| }); |
Contributor
There was a problem hiding this comment.
Regex pattern has issues.
The regex / Jacket/ contains a leading space and capital "J", but response.toLowerCase() ensures the response is lowercase, so "Jacket" will never match. Also, "precipitation" is unlikely to appear in the current implementation.
Proposed fix
// Should mention weather-relevant items
- expect(response.toLowerCase()).toMatch(/rain|wet|precipitation|rainy|coat| Jacket|umbrella/);
+ expect(response.toLowerCase()).toMatch(/rain|wet|gear|layers/);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(response).toBeDefined(); | |
| // Should mention weather-relevant items | |
| expect(response.toLowerCase()).toMatch(/rain|wet|precipitation|rainy|coat| Jacket|umbrella/); | |
| }); | |
| expect(response).toBeDefined(); | |
| // Should mention weather-relevant items | |
| expect(response.toLowerCase()).toMatch(/rain|wet|gear|layers/); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/offline-ai/__tests__/offline-ai.test.ts` around lines 55 -
58, The test's regex in the expect(response.toLowerCase()).toMatch(...) is
incorrect: it contains a leading space before "Jacket" and a capital "J" which
will never match after toLowerCase(), and includes "precipitation" which is
unlikely; update the pattern to use all-lowercase tokens and remove the stray
space (e.g., replace the current pattern with
/(rain|wet|rainy|coat|jacket|umbrella)/) in the
expect(response.toLowerCase()).toMatch(...) assertion referencing the response
variable.
Comment on lines
+21
to
+24
| export interface GenerateOptions { | ||
| context?: LLMContext; | ||
| systemPrompt?: string; | ||
| } |
Contributor
There was a problem hiding this comment.
systemPrompt is defined but never used.
The GenerateOptions interface declares systemPrompt but generate() ignores it entirely. Either implement support for it or remove the option to avoid a misleading API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/offline-ai/lib/MockLLMProvider.ts` around lines 21 - 24,
GenerateOptions declares systemPrompt but MockLLMProvider.generate ignores it;
either remove systemPrompt from the interface or wire it into the generate
flow—update the generate method in MockLLMProvider to accept the options param
(GenerateOptions) and incorporate options.systemPrompt when constructing the LLM
response (e.g., prepend or merge it into the prompt/context handling in
generate()), or delete systemPrompt from the GenerateOptions type and all call
sites to keep the API accurate; reference the GenerateOptions interface and the
generate() method in MockLLMProvider when making the change.
Contributor
Author
|
👋 @copilot |
Contributor
Author
|
@copilot |
3 similar comments
Contributor
Author
|
@copilot |
Contributor
Author
|
@copilot |
Contributor
Author
|
@copilot |
Collaborator
|
👋 @copilot |
Contributor
|
@andrew-bierman I've opened a new pull request, #1871, to work on those changes. Once the pull request is ready, I'll request review from you. |
5 tasks
Collaborator
Contributor
|
@andrew-bierman I've opened a new pull request, #1874, to work on those changes. Once the pull request is ready, I'll request review from you. |
Collaborator
Contributor
|
@andrew-bierman I've opened a new pull request, #1876, to work on those changes. Once the pull request is ready, I'll request review from you. |
Collaborator
Contributor
|
@andrew-bierman I've opened a new pull request, #1877, to work on those changes. Once the pull request is ready, I'll request review from you. |
3 tasks
Collaborator
Contributor
|
@andrew-bierman I've opened a new pull request, #1878, to work on those changes. Once the pull request is ready, I'll request review from you. |
4 tasks
Collaborator
Contributor
|
@andrew-bierman I've opened a new pull request, #1915, to work on those changes. Once the pull request is ready, I'll request review from you. |
12 tasks
Collaborator
5 tasks
Contributor
|
@andrew-bierman I've opened a new pull request, #1920, to work on those changes. Once the pull request is ready, I'll request review from you. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/expo/features/offline-ai/__tests__/offline-ai.test.ts (1)
1-2:⚠️ Potential issue | 🟡 MinorRun Biome organize-imports on this file.
Line 1 is still failing the Biome check for import ordering, so the PR stays red until this file is re-organized.
Based on learnings, "Imports are auto-organized by Biome — do not manually sort them".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/offline-ai/__tests__/offline-ai.test.ts` around lines 1 - 2, Run Biome's automatic import organizer on this test file instead of manually reordering imports: revert any hand-sorted import lines and run the Biome organize-imports command so the imports for describe/it/expect/beforeEach and MockLLMProvider/LLMContext are auto-arranged, ensuring the top-of-file import block matches Biome's expected ordering.
🧹 Nitpick comments (1)
apps/expo/features/offline-ai/__tests__/offline-ai.test.ts (1)
91-105: Rename this case or model an actual multi-trail input.The current fixture only contains a single
trail, so the description is misleading. Either rename the test to match the setup or expand the context shape if multi-trail handling is part of the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/offline-ai/__tests__/offline-ai.test.ts` around lines 91 - 105, The test "should process multiple trails in context" is misleading because the LLMContext fixture only contains a single trail; either rename the test to reflect single-trail behavior (update the it(...) description) or modify the fixture to represent multiple trails (e.g., change the LLMContext shape to include an array of trails or additional trail objects) and ensure provider.generate is invoked with the updated context; update assertions (expect(response)...) accordingly so the test name, LLMContext variable, and provider.generate call are consistent.
🤖 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/__tests__/offline-ai.test.ts`:
- Around line 19-35: The test only asserts trail.name is present; update the
tests around the LLMContext use in offline-ai.test.ts (the cases invoking
provider.generate and the LLMContext with trail and activity) to also assert the
other fields are represented: check that response contains trail.difficulty,
trail.length (formatted or numeric as expected), and activity; alternatively add
a baseline call with a context lacking trail/activity and assert the enriched
response differs, ensuring provider.generate includes difficulty/length/activity
and not just trail.name.
- Around line 66-72: Update the test so it verifies the systemPrompt actually
affects output by calling provider.generate twice with the same input
('Hello')—once with systemPrompt: 'You are a helpful hiking assistant.' and once
without—and asserting the two responses differ (e.g.,
expect(responseWithPrompt).not.toEqual(responseWithoutPrompt)); use the existing
generate function and response variables (response,
responseWithPrompt/responseWithoutPrompt) to perform the comparison so the test
fails if the prompt is ignored.
---
Duplicate comments:
In `@apps/expo/features/offline-ai/__tests__/offline-ai.test.ts`:
- Around line 1-2: Run Biome's automatic import organizer on this test file
instead of manually reordering imports: revert any hand-sorted import lines and
run the Biome organize-imports command so the imports for
describe/it/expect/beforeEach and MockLLMProvider/LLMContext are auto-arranged,
ensuring the top-of-file import block matches Biome's expected ordering.
---
Nitpick comments:
In `@apps/expo/features/offline-ai/__tests__/offline-ai.test.ts`:
- Around line 91-105: The test "should process multiple trails in context" is
misleading because the LLMContext fixture only contains a single trail; either
rename the test to reflect single-trail behavior (update the it(...)
description) or modify the fixture to represent multiple trails (e.g., change
the LLMContext shape to include an array of trails or additional trail objects)
and ensure provider.generate is invoked with the updated context; update
assertions (expect(response)...) accordingly so the test name, LLMContext
variable, and provider.generate call are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8074d4bd-0edd-49bc-a61e-830decc8c003
📒 Files selected for processing (2)
apps/expo/features/offline-ai/__tests__/offline-ai.test.tsapps/expo/features/offline-ai/lib/MockLLMProvider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/expo/features/offline-ai/lib/MockLLMProvider.ts
Comment on lines
+19
to
+35
| it('should include trail name in response when trail context is provided', async () => { | ||
| const context: LLMContext = { | ||
| trail: { | ||
| name: 'Test Trail', | ||
| difficulty: 'moderate', | ||
| length: 5.2, | ||
| }, | ||
| activity: 'hiking', | ||
| }; | ||
|
|
||
| const response = await provider.generate( | ||
| 'What gear do I need for this trail?', | ||
| { context } | ||
| ); | ||
|
|
||
| expect(response).toContain('Test Trail'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert the fields these cases are meant to cover.
Both tests only prove that the trail name is surfaced. They still pass if difficulty, length, or activity are ignored entirely, which leaves the PR's main behavior under-tested. Compare against a baseline response without those fields, or assert on activity/metadata-specific output.
Possible tightening
it('should include trail name in response when trail context is provided', async () => {
+ const baseline = await provider.generate(
+ 'What gear do I need for this trail?',
+ { context: { trail: { name: 'Test Trail' } } }
+ );
+
const context: LLMContext = {
trail: {
name: 'Test Trail',
difficulty: 'moderate',
length: 5.2,
},
activity: 'hiking',
};
@@
expect(response).toContain('Test Trail');
+ expect(response).not.toBe(baseline);
});
@@
it('should incorporate activity context into recommendations', async () => {
+ const baseline = await provider.generate('What do I need?', {
+ context: { trail: { name: 'Lakeside Camp' } },
+ });
+
const context: LLMContext = {
activity: 'camping',
trail: {
name: 'Lakeside Camp',
},
@@
expect(response).toContain('Lakeside Camp');
+ expect(response).not.toBe(baseline);
});Also applies to: 74-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/offline-ai/__tests__/offline-ai.test.ts` around lines 19 -
35, The test only asserts trail.name is present; update the tests around the
LLMContext use in offline-ai.test.ts (the cases invoking provider.generate and
the LLMContext with trail and activity) to also assert the other fields are
represented: check that response contains trail.difficulty, trail.length
(formatted or numeric as expected), and activity; alternatively add a baseline
call with a context lacking trail/activity and assert the enriched response
differs, ensuring provider.generate includes difficulty/length/activity and not
just trail.name.
Comment on lines
+66
to
+72
| it('should use system prompt when provided', async () => { | ||
| const response = await provider.generate('Hello', { | ||
| systemPrompt: 'You are a helpful hiking assistant.', | ||
| }); | ||
|
|
||
| expect(response).toBeDefined(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Make the systemPrompt test prove the prompt is used.
Right now this case only checks that generate() returns something. It won't fail if systemPrompt is ignored. A simple black-box check is to compare the response with and without the prompt and assert they differ.
Possible tightening
it('should use system prompt when provided', async () => {
+ const baseline = await provider.generate('Hello');
const response = await provider.generate('Hello', {
systemPrompt: 'You are a helpful hiking assistant.',
});
expect(response).toBeDefined();
+ expect(response).not.toBe(baseline);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/offline-ai/__tests__/offline-ai.test.ts` around lines 66 -
72, Update the test so it verifies the systemPrompt actually affects output by
calling provider.generate twice with the same input ('Hello')—once with
systemPrompt: 'You are a helpful hiking assistant.' and once without—and
asserting the two responses differ (e.g.,
expect(responseWithPrompt).not.toEqual(responseWithoutPrompt)); use the existing
generate function and response variables (response,
responseWithPrompt/responseWithoutPrompt) to perform the comparison so the test
fails if the prompt is ignored.
- Fixed MockLLMProvider.generate() to properly process trail context - Added support for weather, activity, and trail information in responses - Updated tests to verify context-aware responses Fixes #1864
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
Squashed refinements from the stalled sub-PR #1915 onto the current fix/1864-offline-ai-mock-llm-provider tip (which already includes #1920). - Sorted imports and trailing commas in test file - Renamed tests to reflect single-trail scope and document that systemPrompt is accepted but not applied in the mock - JSDoc on GenerateOptions.systemPrompt - Normalize conditions.toLowerCase() into a local variable - Only append ", " separator when activity/weather follows the trail fragment (removes trailing comma on trail-only responses) - trail.length !== undefined check preserves zero-length trails - Template literal → plain string, drop dead fallback return - Preserve the stronger weather regex added in #1920
andrew-bierman
force-pushed
the
fix/1864-offline-ai-mock-llm-provider
branch
from
b08b645 to
4d86940
Compare
This was referenced Apr 11, 2026
andrew-bierman
added a commit
that referenced
this pull request
May 14, 2026
…-provider fix(offline-ai): fix MockLLMProvider to process trail context
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixed issue #1864 where MockLLMProvider.generate() was returning a hardcoded greeting instead of processing trail context.
Changes Made
apps/expo/features/offline-ai/lib/MockLLMProvider.tswith proper context processingapps/expo/features/offline-ai/__tests__/offline-ai.test.tswith tests that verify context-aware responsesTest Results
All 8 tests pass
Steps to Test
bun test apps/expo/features/offline-ai/__tests__/offline-ai.test.ts🤖 Created via Pearl
Summary by CodeRabbit
New Features
Tests
👋 @copilot