feat: LLM coaching with Groq + settings UI + code cleanup#30
Conversation
📝 WalkthroughWalkthroughAdds Groq LLM rephrasing for coaching prompts with secure API-key storage using Electron safeStorage, updates prompt pipeline to call the rephraser asynchronously, exposes IPC/preload methods and a renderer settings panel for key management, and adds Playwright/Vitest test suites and configs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Renderer as Renderer (hub.js)
participant Preload as Preload Bridge
participant Main as Main Process
participant Storage as Electron safeStorage
participant CoachModule as coach.ts
User->>Renderer: Open settings panel
Renderer->>Preload: getApiKey()
Preload->>Main: IPC: 'get-api-key'
Main->>Storage: Read encrypted groqApiKey
Main->>Main: Decrypt via safeStorage
Main-->>Preload: Return decrypted key (or '')
Preload-->>Renderer: Return key string
Renderer->>Renderer: Populate input, update badge
User->>Renderer: Enter API key & click Save
Renderer->>Preload: setApiKey(key)
Preload->>Main: IPC: 'set-api-key'
Main->>Storage: Encrypt & store groqApiKey (or delete)
Main->>CoachModule: Call setGroqApiKey(key)
CoachModule->>CoachModule: Initialize/refresh Groq client
Main-->>Preload: Resolve
Preload-->>Renderer: Return void
Renderer->>Renderer: Show success, update badge
sequenceDiagram
participant GameEngine as Game State
participant Prompts as prompts.ts
participant Templates as templates/detectors
participant Coach as coach.ts
participant Groq as Groq API
GameEngine->>Prompts: evaluate(state, snapshot)
Prompts->>Templates: pickTemplateFallback(category, data)
Templates-->>Prompts: Resolved template text
Prompts->>Coach: rephrasePrompt(template, snapshot, phase)
Coach->>Groq: chat completions request (system prompt)
Groq-->>Coach: Single-sentence rephrase
Coach->>Coach: Apply recency window, return text or null
Prompts->>Prompts: Use rephrased or fallback template
Prompts-->>GameEngine: Final coaching prompt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/coach.ts`:
- Around line 93-100: Validate the model output in the block that reads
response.choices[0]?.message?.content before adding to recentPhrases: compute
text by trimming, then reject (set text to null and do not push to
recentPhrases) if it contains more than one sentence (e.g., more than one
sentence-ending punctuation like '.', '!', '?' sequence) or more than 15 words
(split on whitespace to count words); only when it passes both checks should you
unshift into recentPhrases and enforce MAX_RECENT; finally return the validated
text (or null on failure).
- Around line 80-91: The Groq chat call in client.chat.completions.create is
using the SDK default (1 minute) which is too long for live coaching; update the
call to enforce a much shorter timeout (about 2000–3000 ms) so responses arrive
within the live game window—either pass a per-request timeout option on
client.chat.completions.create or configure the client instance-level timeout
used by that call (refer to client and the chat.completions.create invocation in
src/main/coach.ts) and ensure the new timeout value (2–3 seconds) is applied and
tested.
In `@src/main/main.ts`:
- Around line 125-133: The else branch that deletes the stored key via
store.delete('groqApiKey') doesn't clear the in-memory/active API key; update
the ipcMain.handle('set-api-key', ...) handler so that when key is falsy you
also call setGroqApiKey with an empty value (e.g., setGroqApiKey('') or null per
its contract) after store.delete('groqApiKey') to clear the runtime key; keep
the existing encrypted/write path unchanged.
- Around line 125-129: The set-api-key IPC handler currently calls
safeStorage.encryptString() without guarding for unavailable OS encryption;
update the 'set-api-key' handler to either check
safeStorage.isEncryptionAvailable() before calling safeStorage.encryptString()
or wrap the encrypt call in a try/catch and fall back to storing the raw key (or
a clear fallback behavior) to mirror the get-api-key decryption pattern; adjust
uses of safeStorage.encryptString, store.set('groqApiKey', ...), and
setGroqApiKey(...) accordingly so failures are handled gracefully and errors are
logged or communicated.
- Around line 115-123: The IPC handler registered with
ipcMain.handle('get-api-key') currently decrypts and returns the plaintext via
safeStorage.decryptString after reading store.get('groqApiKey'); change this to
only return a boolean: check whether an encrypted value exists and is
decryptable, but do NOT return the decrypted secret—return true when a valid key
exists and false otherwise. Update the try/catch around
safeStorage.decryptString to only detect success/failure and return true/false
accordingly, and adjust any callers expecting the key to use a different IPC
channel if they need the secret.
In `@src/main/prompts.ts`:
- Around line 121-129: The evaluate function can leave inCooldown true if
pickPrompt throws; wrap the await pickPrompt(snapshot) call in a try-catch
inside evaluate, and in the catch set inCooldown = false and handle/log the
error (then return) so the cooldown is always cleared on exceptions; keep the
rest of evaluate behavior unchanged and only modify the block around pickPrompt
to ensure inCooldown is reset on error.
In `@src/renderer/hub.css`:
- Line 261: The font-family declaration in hub.css uses quoted font name
'Rajdhani' which violates the stylelint rule font-family-name-quotes; update the
CSS rule (the font-family line in src/renderer/hub.css) to remove the quotes so
it reads font-family: Rajdhani, sans-serif; ensuring the unquoted font name
fixes the linter error.
In `@src/renderer/hub.html`:
- Around line 52-67: The two buttons missing explicit types (elements with id
"save-key-btn" and "settings-btn") default to type="submit" in forms; update
each <button> element to include type="button" to prevent implicit form
submission (add type="button" to the button with id "save-key-btn" and to the
button with id "settings-btn").
- Around line 44-51: The input with id "api-key-input" has no accessible name;
add an accessible name by adding aria-label="Groq API key" to the input element
(or alternatively associate a <label> with id "api-key-input") so screen readers
can identify the API key field; update the element referenced by id
"api-key-input" accordingly.
In `@src/renderer/hub.js`:
- Around line 16-21: Wrap the initial API-key fetch
(window.electronAPI.getApiKey()) in a try/catch so a rejection doesn't abort
startup; if the call succeeds set apiKeyInput.placeholder =
KEY_SAVED_PLACEHOLDER and set keyBadge.textContent = 'active' and
keyBadge.className = 'key-badge active', but if it throws or returns falsy set
the UI to the inactive state (e.g., keyBadge.textContent = 'inactive' and
keyBadge.className = 'key-badge inactive' and leave apiKeyInput as editable) and
let execution continue so the settings listeners still get registered; update
the block around the getApiKey call accordingly.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a6abb1c-e626-4c58-b6e4-e8d8e8f2cfe1
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonriotgames.pemis excluded by!**/*.pem
📒 Files selected for processing (12)
package.jsonsrc/main/coach.tssrc/main/context.test.tssrc/main/context.tssrc/main/detectors.tssrc/main/main.tssrc/main/prompts.tssrc/preload/preload.tssrc/renderer/hub.csssrc/renderer/hub.htmlsrc/renderer/hub.jssrc/types.ts
| const response = await client.chat.completions.create({ | ||
| model: 'llama-3.3-70b-versatile', | ||
| messages: [ | ||
| { role: 'system', content: SYSTEM_PROMPT }, | ||
| { | ||
| role: 'user', | ||
| content: `Game time: ${minutes}:${seconds} (${phase.replace('_', ' ')})\n${playerContext}\n\nRephrase this: "${basePrompt}"${recentBlock}`, | ||
| }, | ||
| ], | ||
| max_tokens: 35, | ||
| temperature: 0.7, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure
git ls-files | head -20Repository: Ilia01/Beacon
Length of output: 429
🏁 Script executed:
# Check if the file exists and get its size
wc -l src/main/coach.tsRepository: Ilia01/Beacon
Length of output: 78
🏁 Script executed:
# Read the relevant section around lines 80-91
sed -n '70,110p' src/main/coach.tsRepository: Ilia01/Beacon
Length of output: 960
🏁 Script executed:
# Look for how the Groq client is initialized to understand default timeouts
rg "client\s*=" src/main/coach.ts -A 5Repository: Ilia01/Beacon
Length of output: 310
🏁 Script executed:
# Check for timeout patterns elsewhere in the codebase
rg "Promise\.race|timeout|TIMEOUT_MS" src/ -iRepository: Ilia01/Beacon
Length of output: 490
🏁 Script executed:
# Look for getClient function definition
rg "function getClient|const getClient|export.*getClient" src/ -A 10Repository: Ilia01/Beacon
Length of output: 594
🏁 Script executed:
# Check if there's Groq client initialization or configuration
rg "Groq|groq" src/ -i -B 2 -A 5Repository: Ilia01/Beacon
Length of output: 3696
🏁 Script executed:
# Read the beginning of coach.ts to see imports and client setup
head -40 src/main/coach.tsRepository: Ilia01/Beacon
Length of output: 1106
🏁 Script executed:
# Check package.json for Groq dependency and version
cat package.json | grep -A 2 -B 2 groqRepository: Ilia01/Beacon
Length of output: 144
🏁 Script executed:
# Check where rephrasePrompt is called from
rg "rephrasePrompt" src/ -B 2 -A 2Repository: Ilia01/Beacon
Length of output: 881
🏁 Script executed:
# Look at the full rephrasePrompt function
sed -n '50,104p' src/main/coach.tsRepository: Ilia01/Beacon
Length of output: 1696
🏁 Script executed:
# Check Groq SDK documentation defaults via web search to understand if there's built-in timeout
cat package.json | grep -E "groq-sdk|node|version"Repository: Ilia01/Beacon
Length of output: 159
🏁 Script executed:
# Look for any configuration or options passed when creating the Groq client
rg "new Groq|Groq\(" src/ -B 3 -A 3Repository: Ilia01/Beacon
Length of output: 320
🌐 Web query:
Groq SDK JavaScript timeout default configuration groq-sdk
💡 Result:
The default timeout configuration for the Groq JavaScript/TypeScript SDK (groq-sdk) is 1 minute (60,000 milliseconds) for requests. This can be configured via the timeout option when initializing the client or overridden per-request. On timeout, an APIConnectionTimeoutError is thrown, and the request is retried up to 2 times by default. Example from official documentation: const client = new Groq({ timeout: 20 * 1000, // 20 seconds (default is 1 minute) });
Citations:
- 1: https://github.com/groq/groq-typescript
- 2: https://registry.npmjs.org/groq-sdk
- 3: https://www.npmjs.com/package/groq-sdk?activeTab=code
- 4: https://www.npmjs.com/package/groq-sdk/v/0.2.1
🌐 Web query:
groq-sdk chat.completions.create timeout options
💡 Result:
The Groq SDK (both Python 'groq' and TypeScript 'groq-sdk') supports timeout configuration for chat.completions.create calls. Timeouts are not a direct parameter of the create method but are set at the client level or overridden per-request. For Python (groq library): - Default: 60 seconds total timeout (1 minute), with 5-second connect timeout. - Client initialization: client = Groq(timeout=20.0) or timeout=httpx.Timeout(60.0, read=5.0, write=10.0, connect=2.0) - Per-request override: client.with_options(timeout=5.0).chat.completions.create(...) - On timeout: APITimeoutError is raised; timeouts are retried twice by default. Example: from groq import Groq import httpx client = Groq(timeout=httpx.Timeout(60.0, read=30.0, connect=5.0)) chat_completion = client.chat.completions.create( messages=[{"role": "user", "content": "Hello"}], model="llama3-8b-8192" ) For TypeScript (groq-sdk): - Default: 1 minute. - Client: new Groq({ timeout: 20 * 1000 }) - Per-request: client.chat.completions.create(params, { timeout: 5 * 1000 }) - On timeout: APIConnectionTimeoutError; retried twice by default. The API endpoint itself (/openai/v1/chat/completions) has no timeout parameter. Use longer timeouts for streaming or long generations. Install via pip install groq or npm install groq-sdk.
Citations:
- 1: https://github.com/groq/groq-python
- 2: https://github.com/groq/groq-python/blob/main/README.md
- 3: https://github.com/groq/groq-typescript
- 4: https://leeroopedia.com/index.php/Heuristic:Groq_Groq_python_Timeout_Configuration
- 5: https://console.groq.com/docs/libraries
- 6: https://pypi.org/project/groq/
Set a tighter timeout for Groq API calls—1 minute is too long for live game coaching.
The SDK has a 1-minute default timeout, but by the time a response arrives, the game moment has passed. Set this to 2–3 seconds to keep prompts responsive.
Proposed fix
+const GROQ_TIMEOUT_MS = 2500;
+
export async function rephrasePrompt(
basePrompt: string,
snapshot: GameSnapshot,
phase: GamePhase,
): Promise<string | null> {
@@
try {
- const response = await client.chat.completions.create({
- model: 'llama-3.3-70b-versatile',
- messages: [
- { role: 'system', content: SYSTEM_PROMPT },
- {
- role: 'user',
- content: `Game time: ${minutes}:${seconds} (${phase.replace('_', ' ')})\n${playerContext}\n\nRephrase this: "${basePrompt}"${recentBlock}`,
- },
- ],
- max_tokens: 35,
- temperature: 0.7,
- });
+ const response = (await Promise.race([
+ client.chat.completions.create({
+ model: 'llama-3.3-70b-versatile',
+ messages: [
+ { role: 'system', content: SYSTEM_PROMPT },
+ {
+ role: 'user',
+ content: `Game time: ${minutes}:${seconds} (${phase.replace('_', ' ')})\n${playerContext}\n\nRephrase this: "${basePrompt}"${recentBlock}`,
+ },
+ ],
+ max_tokens: 35,
+ temperature: 0.7,
+ }),
+ new Promise<never>((_, reject) =>
+ setTimeout(() => reject(new Error('Groq timeout')), GROQ_TIMEOUT_MS),
+ ),
+ ])) as Awaited<ReturnType<typeof client.chat.completions.create>>;📝 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.
| const response = await client.chat.completions.create({ | |
| model: 'llama-3.3-70b-versatile', | |
| messages: [ | |
| { role: 'system', content: SYSTEM_PROMPT }, | |
| { | |
| role: 'user', | |
| content: `Game time: ${minutes}:${seconds} (${phase.replace('_', ' ')})\n${playerContext}\n\nRephrase this: "${basePrompt}"${recentBlock}`, | |
| }, | |
| ], | |
| max_tokens: 35, | |
| temperature: 0.7, | |
| }); | |
| const response = (await Promise.race([ | |
| client.chat.completions.create({ | |
| model: 'llama-3.3-70b-versatile', | |
| messages: [ | |
| { role: 'system', content: SYSTEM_PROMPT }, | |
| { | |
| role: 'user', | |
| content: `Game time: ${minutes}:${seconds} (${phase.replace('_', ' ')})\n${playerContext}\n\nRephrase this: "${basePrompt}"${recentBlock}`, | |
| }, | |
| ], | |
| max_tokens: 35, | |
| temperature: 0.7, | |
| }), | |
| new Promise<never>((_, reject) => | |
| setTimeout(() => reject(new Error('Groq timeout')), GROQ_TIMEOUT_MS), | |
| ), | |
| ])) as Awaited<ReturnType<typeof client.chat.completions.create>>; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/coach.ts` around lines 80 - 91, The Groq chat call in
client.chat.completions.create is using the SDK default (1 minute) which is too
long for live coaching; update the call to enforce a much shorter timeout (about
2000–3000 ms) so responses arrive within the live game window—either pass a
per-request timeout option on client.chat.completions.create or configure the
client instance-level timeout used by that call (refer to client and the
chat.completions.create invocation in src/main/coach.ts) and ensure the new
timeout value (2–3 seconds) is applied and tested.
| const text = response.choices[0]?.message?.content?.trim() ?? null; | ||
| if (text) { | ||
| recentPhrases.unshift(text); | ||
| if (recentPhrases.length > MAX_RECENT) { | ||
| recentPhrases.pop(); | ||
| } | ||
| } | ||
| return text; |
There was a problem hiding this comment.
Enforce output constraints before returning model text.
Model output is trusted as-is. If it violates the 1-sentence/15-word constraint or adds extra content, it still gets surfaced to users. Validate locally and fall back to null on invalid output.
Proposed fix
const MAX_RECENT = 6;
+const MAX_WORDS = 15;
+
+function isValidRephrase(text: string): boolean {
+ const wordCount = text.trim().split(/\s+/).filter(Boolean).length;
+ const sentenceCount = text.split(/[.!?]+/).filter((s) => s.trim().length > 0).length;
+ return wordCount > 0 && wordCount <= MAX_WORDS && sentenceCount <= 1;
+}
@@
- const text = response.choices[0]?.message?.content?.trim() ?? null;
- if (text) {
+ const text = response.choices[0]?.message?.content?.trim() ?? null;
+ if (!text || !isValidRephrase(text)) return null;
+ {
recentPhrases.unshift(text);
if (recentPhrases.length > MAX_RECENT) {
recentPhrases.pop();
}
}📝 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.
| const text = response.choices[0]?.message?.content?.trim() ?? null; | |
| if (text) { | |
| recentPhrases.unshift(text); | |
| if (recentPhrases.length > MAX_RECENT) { | |
| recentPhrases.pop(); | |
| } | |
| } | |
| return text; | |
| const MAX_RECENT = 6; | |
| const MAX_WORDS = 15; | |
| function isValidRephrase(text: string): boolean { | |
| const wordCount = text.trim().split(/\s+/).filter(Boolean).length; | |
| const sentenceCount = text.split(/[.!?]+/).filter((s) => s.trim().length > 0).length; | |
| return wordCount > 0 && wordCount <= MAX_WORDS && sentenceCount <= 1; | |
| } | |
| const text = response.choices[0]?.message?.content?.trim() ?? null; | |
| if (!text || !isValidRephrase(text)) return null; | |
| { | |
| recentPhrases.unshift(text); | |
| if (recentPhrases.length > MAX_RECENT) { | |
| recentPhrases.pop(); | |
| } | |
| } | |
| return text; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/coach.ts` around lines 93 - 100, Validate the model output in the
block that reads response.choices[0]?.message?.content before adding to
recentPhrases: compute text by trimming, then reject (set text to null and do
not push to recentPhrases) if it contains more than one sentence (e.g., more
than one sentence-ending punctuation like '.', '!', '?' sequence) or more than
15 words (split on whitespace to count words); only when it passes both checks
should you unshift into recentPhrases and enforce MAX_RECENT; finally return the
validated text (or null on failure).
| ipcMain.handle('get-api-key', () => { | ||
| const encrypted = store.get('groqApiKey') as string | undefined; | ||
| if (!encrypted) return ''; | ||
| try { | ||
| return safeStorage.decryptString(Buffer.from(encrypted, 'base64')); | ||
| } catch { | ||
| return ''; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the full context of src/main/main.ts around lines 115-123
fd -t f "main.ts" | grep -E "src/main/main\.ts$"Repository: Ilia01/Beacon
Length of output: 73
🏁 Script executed:
# Check the file size and examine the relevant sections
wc -l src/main/main.tsRepository: Ilia01/Beacon
Length of output: 77
🏁 Script executed:
# Read the IPC handler section and surrounding code
sed -n '110,150p' src/main/main.tsRepository: Ilia01/Beacon
Length of output: 1301
🏁 Script executed:
# Search for all IPC handlers in the file to understand the pattern
rg "ipcMain\.(handle|on)" src/main/main.ts -A 5Repository: Ilia01/Beacon
Length of output: 849
🏁 Script executed:
# Check if hub.js or any renderer calls 'get-api-key'
fd -t f "hub.js" -o -name "*hub*" | head -5Repository: Ilia01/Beacon
Length of output: 287
🏁 Script executed:
# Search for 'get-api-key' usage across the codebase
rg "get-api-key" --type ts --type jsRepository: Ilia01/Beacon
Length of output: 207
🏁 Script executed:
# Check safeStorage usage and error handling patterns
rg "safeStorage\.(encrypt|decrypt)" src/main/main.ts -B 2 -A 3Repository: Ilia01/Beacon
Length of output: 605
🏁 Script executed:
# Find renderer/UI files that might call getApiKey
fd -t f "\.(ts|tsx|js|jsx)$" src | grep -vE "src/main/" | head -20Repository: Ilia01/Beacon
Length of output: 172
🏁 Script executed:
# Search for where getApiKey is invoked in the codebase
rg "getApiKey" --type ts --type js -B 2 -A 3Repository: Ilia01/Beacon
Length of output: 765
🏁 Script executed:
# Look for setGroqApiKey function definition
rg "setGroqApiKey|function setGroqApiKey" src/main/main.ts -B 2 -A 8Repository: Ilia01/Beacon
Length of output: 1038
🏁 Script executed:
# Check if there's any safeStorage.isEncryptionAvailable check
rg "isEncryptionAvailable" src/main/main.tsRepository: Ilia01/Beacon
Length of output: 39
🏁 Script executed:
# Check coach.ts for key management functions
fd -t f "coach.ts" | xargs cat | head -100Repository: Ilia01/Beacon
Length of output: 2833
Return a boolean instead of the decrypted API key.
The handler exposes the plaintext secret over IPC. The renderer only needs a status indicator (whether a key exists) to display the "active" badge; send true or false instead of the actual key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/main.ts` around lines 115 - 123, The IPC handler registered with
ipcMain.handle('get-api-key') currently decrypts and returns the plaintext via
safeStorage.decryptString after reading store.get('groqApiKey'); change this to
only return a boolean: check whether an encrypted value exists and is
decryptable, but do NOT return the decrypted secret—return true when a valid key
exists and false otherwise. Update the try/catch around
safeStorage.decryptString to only detect success/failure and return true/false
accordingly, and adjust any callers expecting the key to use a different IPC
channel if they need the secret.
| ipcMain.handle('set-api-key', (_event, key: string) => { | ||
| if (key) { | ||
| const encrypted = safeStorage.encryptString(key).toString('base64'); | ||
| store.set('groqApiKey', encrypted); | ||
| setGroqApiKey(key); | ||
| } else { | ||
| store.delete('groqApiKey'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Clearing the stored key does not clear the active key in memory.
Line 131 deletes from storage, but the previously set runtime key remains active because setGroqApiKey is not called in the empty-key branch. Clear the runtime key in the else branch.
Proposed fix
ipcMain.handle('set-api-key', (_event, key: string) => {
if (key) {
const encrypted = safeStorage.encryptString(key).toString('base64');
store.set('groqApiKey', encrypted);
setGroqApiKey(key);
} else {
store.delete('groqApiKey');
+ setGroqApiKey('');
}
});📝 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.
| ipcMain.handle('set-api-key', (_event, key: string) => { | |
| if (key) { | |
| const encrypted = safeStorage.encryptString(key).toString('base64'); | |
| store.set('groqApiKey', encrypted); | |
| setGroqApiKey(key); | |
| } else { | |
| store.delete('groqApiKey'); | |
| } | |
| }); | |
| ipcMain.handle('set-api-key', (_event, key: string) => { | |
| if (key) { | |
| const encrypted = safeStorage.encryptString(key).toString('base64'); | |
| store.set('groqApiKey', encrypted); | |
| setGroqApiKey(key); | |
| } else { | |
| store.delete('groqApiKey'); | |
| setGroqApiKey(''); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/main.ts` around lines 125 - 133, The else branch that deletes the
stored key via store.delete('groqApiKey') doesn't clear the in-memory/active API
key; update the ipcMain.handle('set-api-key', ...) handler so that when key is
falsy you also call setGroqApiKey with an empty value (e.g., setGroqApiKey('')
or null per its contract) after store.delete('groqApiKey') to clear the runtime
key; keep the existing encrypted/write path unchanged.
| ipcMain.handle('set-api-key', (_event, key: string) => { | ||
| if (key) { | ||
| const encrypted = safeStorage.encryptString(key).toString('base64'); | ||
| store.set('groqApiKey', encrypted); | ||
| setGroqApiKey(key); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify safeStorage usage and missing availability checks.
# Expected: encrypt/decrypt calls exist; no isEncryptionAvailable guard near set-api-key.
rg -n "safeStorage\\.(encryptString|decryptString|isEncryptionAvailable)" src/main/main.ts -C 3
rg -n "ipcMain\\.handle\\('set-api-key'|ipcMain\\.handle\\('get-api-key'" src/main/main.ts -C 8
# Verify renderer-side handling of rejected key-save operations.
# Expected: inspect whether setApiKey invocation is wrapped with try/catch.
rg -n "setApiKey\\(|getApiKey\\(" src/renderer/hub.js src/preload/preload.ts -C 4Repository: Ilia01/Beacon
Length of output: 2729
Add encryption availability check or wrap encryption in try/catch.
The set-api-key handler encrypts without guards (line 127), but the get-api-key handler wraps decryption in try/catch. If OS encryption is unavailable, safeStorage.encryptString() throws. Either check safeStorage.isEncryptionAvailable() first or wrap the encryption call in try/catch to match the pattern used elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/main.ts` around lines 125 - 129, The set-api-key IPC handler
currently calls safeStorage.encryptString() without guarding for unavailable OS
encryption; update the 'set-api-key' handler to either check
safeStorage.isEncryptionAvailable() before calling safeStorage.encryptString()
or wrap the encrypt call in a try/catch and fall back to storing the raw key (or
a clear fallback behavior) to mirror the get-api-key decryption pattern; adjust
uses of safeStorage.encryptString, store.set('groqApiKey', ...), and
setGroqApiKey(...) accordingly so failures are handled gracefully and errors are
logged or communicated.
| async function evaluate(snapshot: GameSnapshot, win: BrowserWindow) { | ||
| if (paused || inCooldown) return; | ||
| inCooldown = true; | ||
|
|
||
| const prompt = pickPrompt(snapshot); | ||
| if (!prompt) return; | ||
| const prompt = await pickPrompt(snapshot); | ||
| if (!prompt) { | ||
| inCooldown = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "prompts.ts" -type f | head -20Repository: Ilia01/Beacon
Length of output: 78
🏁 Script executed:
find . -path "*src/main/prompts.ts" -type fRepository: Ilia01/Beacon
Length of output: 78
🏁 Script executed:
cat -n src/main/prompts.ts | head -150Repository: Ilia01/Beacon
Length of output: 5731
🏁 Script executed:
rg "evaluate\(" src/main/prompts.ts -A 2 -B 2Repository: Ilia01/Beacon
Length of output: 384
🏁 Script executed:
rg "evaluate" . --type ts | grep -v node_modules | head -20Repository: Ilia01/Beacon
Length of output: 264
🏁 Script executed:
rg "\bevaluate\b" . -t ts --glob "!node_modules" | grep -v "Binary"Repository: Ilia01/Beacon
Length of output: 264
🏁 Script executed:
cat -n src/main/prompts.ts | sed -n '140,200p'Repository: Ilia01/Beacon
Length of output: 1350
🏁 Script executed:
rg "export.*rephrasePrompt|function rephrasePrompt" . -t ts | head -5Repository: Ilia01/Beacon
Length of output: 114
🏁 Script executed:
rg "export.*deriveContext|function deriveContext" . -t ts | head -5Repository: Ilia01/Beacon
Length of output: 109
🏁 Script executed:
find . -name "coach.ts" -o -name "coach.js" | grep -v node_modulesRepository: Ilia01/Beacon
Length of output: 76
🏁 Script executed:
cat -n src/main/coach.ts | head -80Repository: Ilia01/Beacon
Length of output: 2794
🏁 Script executed:
cat -n src/main/coach.ts | sed -n '59,120p'Repository: Ilia01/Beacon
Length of output: 1670
🏁 Script executed:
cat -n src/main/coach.ts | tail -60Repository: Ilia01/Beacon
Length of output: 2151
🏁 Script executed:
cat -n src/main/context.ts | head -100Repository: Ilia01/Beacon
Length of output: 3485
Add error handling around await pickPrompt() to prevent cooldown from getting stuck.
If pickPrompt throws an exception, inCooldown remains true and blocks all future prompt emissions. Since evaluate() is called fire-and-forget at the call sites (lines 154, 158), unhandled rejections will silently stall the engine.
Wrap the await in try-catch and reset inCooldown on error:
Proposed fix
async function evaluate(snapshot: GameSnapshot, win: BrowserWindow) {
if (paused || inCooldown) return;
inCooldown = true;
- const prompt = await pickPrompt(snapshot);
+ let prompt: string | null = null;
+ try {
+ prompt = await pickPrompt(snapshot);
+ } catch {
+ inCooldown = false;
+ return;
+ }
if (!prompt) {
inCooldown = false;
return;
}📝 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.
| async function evaluate(snapshot: GameSnapshot, win: BrowserWindow) { | |
| if (paused || inCooldown) return; | |
| inCooldown = true; | |
| const prompt = pickPrompt(snapshot); | |
| if (!prompt) return; | |
| const prompt = await pickPrompt(snapshot); | |
| if (!prompt) { | |
| inCooldown = false; | |
| return; | |
| } | |
| async function evaluate(snapshot: GameSnapshot, win: BrowserWindow) { | |
| if (paused || inCooldown) return; | |
| inCooldown = true; | |
| let prompt: string | null = null; | |
| try { | |
| prompt = await pickPrompt(snapshot); | |
| } catch { | |
| inCooldown = false; | |
| return; | |
| } | |
| if (!prompt) { | |
| inCooldown = false; | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/prompts.ts` around lines 121 - 129, The evaluate function can leave
inCooldown true if pickPrompt throws; wrap the await pickPrompt(snapshot) call
in a try-catch inside evaluate, and in the catch set inCooldown = false and
handle/log the error (then return) so the cooldown is always cleared on
exceptions; keep the rest of evaluate behavior unchanged and only modify the
block around pickPrompt to ensure inCooldown is reset on error.
| border: 1px solid var(--border); | ||
| border-radius: 4px; | ||
| color: var(--text-dim); | ||
| font-family: 'Rajdhani', sans-serif; |
There was a problem hiding this comment.
Fix stylelint violation for font-family quoting.
Remove quotes around Rajdhani to satisfy font-family-name-quotes.
Proposed fix
- font-family: 'Rajdhani', sans-serif;
+ font-family: Rajdhani, sans-serif;📝 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.
| font-family: 'Rajdhani', sans-serif; | |
| font-family: Rajdhani, sans-serif; |
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 261-261: Unexpected quotes around "Rajdhani" (font-family-name-quotes)
(font-family-name-quotes)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hub.css` at line 261, The font-family declaration in hub.css
uses quoted font name 'Rajdhani' which violates the stylelint rule
font-family-name-quotes; update the CSS rule (the font-family line in
src/renderer/hub.css) to remove the quotes so it reads font-family: Rajdhani,
sans-serif; ensuring the unquoted font name fixes the linter error.
| <input | ||
| id="api-key-input" | ||
| type="password" | ||
| class="key-input" | ||
| placeholder="gsk_..." | ||
| spellcheck="false" | ||
| autocomplete="off" | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/renderer/hub.html | sed -n '40,70p'Repository: Ilia01/Beacon
Length of output: 1435
🏁 Script executed:
rg "sr-only" src/Repository: Ilia01/Beacon
Length of output: 39
🏁 Script executed:
find src -name "*.css" -o -name "*.scss" | head -20Repository: Ilia01/Beacon
Length of output: 101
🏁 Script executed:
rg "sr-only|visually-hidden|screen-reader" src/renderer/hub.css src/renderer/styles.cssRepository: Ilia01/Beacon
Length of output: 39
🏁 Script executed:
wc -l src/renderer/hub.css src/renderer/styles.cssRepository: Ilia01/Beacon
Length of output: 125
🏁 Script executed:
cat -n src/renderer/hub.css | head -100Repository: Ilia01/Beacon
Length of output: 2568
🏁 Script executed:
cat -n src/renderer/hub.css | sed -n '100,250p'Repository: Ilia01/Beacon
Length of output: 3794
🏁 Script executed:
cat -n src/renderer/hub.css | sed -n '250,399p'Repository: Ilia01/Beacon
Length of output: 3728
Add an accessible name to the API key input.
Line 45 lacks an associated label. Placeholder text is not reliable as an accessible name. Use aria-label="Groq API key" on the input element.
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 45-45: No matching [ label ] tag found.
(input-requires-label)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hub.html` around lines 44 - 51, The input with id
"api-key-input" has no accessible name; add an accessible name by adding
aria-label="Groq API key" to the input element (or alternatively associate a
<label> with id "api-key-input") so screen readers can identify the API key
field; update the element referenced by id "api-key-input" accordingly.
| <button id="save-key-btn" class="save-btn"> | ||
| <span class="save-btn-text">Save</span> | ||
| </button> | ||
| </div> | ||
|
|
||
| <p id="key-status" class="key-status"></p> | ||
| </div> | ||
| </div> | ||
|
|
||
| <footer class="footer"> | ||
| <button id="settings-btn" class="settings-toggle" title="Settings"> | ||
| <svg width="14" height="14" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"> | ||
| <circle cx="12" cy="12" r="3"/> | ||
| <path d="M12 1v2M12 21v2M4.22 4.22l1.42 1.42M18.36 18.36l1.42 1.42M1 12h2M21 12h2M4.22 19.78l1.42-1.42M18.36 5.64l1.42-1.42"/> | ||
| </svg> | ||
| </button> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "hub.html" -type fRepository: Ilia01/Beacon
Length of output: 80
🏁 Script executed:
cat -n src/renderer/hub.html | sed -n '45,70p'Repository: Ilia01/Beacon
Length of output: 1228
Set explicit type="button" on both buttons.
Line 52 and line 62 omit type. Set type="button" to avoid implicit submit behavior.
Proposed fix
- <button id="save-key-btn" class="save-btn">
+ <button id="save-key-btn" class="save-btn" type="button">
<span class="save-btn-text">Save</span>
</button>
@@
- <button id="settings-btn" class="settings-toggle" title="Settings">
+ <button id="settings-btn" class="settings-toggle" title="Settings" type="button">📝 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.
| <button id="save-key-btn" class="save-btn"> | |
| <span class="save-btn-text">Save</span> | |
| </button> | |
| </div> | |
| <p id="key-status" class="key-status"></p> | |
| </div> | |
| </div> | |
| <footer class="footer"> | |
| <button id="settings-btn" class="settings-toggle" title="Settings"> | |
| <svg width="14" height="14" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"> | |
| <circle cx="12" cy="12" r="3"/> | |
| <path d="M12 1v2M12 21v2M4.22 4.22l1.42 1.42M18.36 18.36l1.42 1.42M1 12h2M21 12h2M4.22 19.78l1.42-1.42M18.36 5.64l1.42-1.42"/> | |
| </svg> | |
| </button> | |
| <button id="save-key-btn" class="save-btn" type="button"> | |
| <span class="save-btn-text">Save</span> | |
| </button> | |
| </div> | |
| <p id="key-status" class="key-status"></p> | |
| </div> | |
| </div> | |
| <footer class="footer"> | |
| <button id="settings-btn" class="settings-toggle" title="Settings" type="button"> | |
| <svg width="14" height="14" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"> | |
| <circle cx="12" cy="12" r="3"/> | |
| <path d="M12 1v2M12 21v2M4.22 4.22l1.42 1.42M18.36 18.36l1.42 1.42M1 12h2M21 12h2M4.22 19.78l1.42-1.42M18.36 5.64l1.42-1.42"/> | |
| </svg> | |
| </button> |
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 52-52: The type attribute must be present on elements.
(button-type-require)
[warning] 62-62: The type attribute must be present on
elements.(button-type-require)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hub.html` around lines 52 - 67, The two buttons missing explicit
types (elements with id "save-key-btn" and "settings-btn") default to
type="submit" in forms; update each <button> element to include type="button" to
prevent implicit form submission (add type="button" to the button with id
"save-key-btn" and to the button with id "settings-btn").
| const existing = await window.electronAPI.getApiKey(); | ||
| if (existing) { | ||
| apiKeyInput.placeholder = KEY_SAVED_PLACEHOLDER; | ||
| keyBadge.textContent = 'active'; | ||
| keyBadge.className = 'key-badge active'; | ||
| } |
There was a problem hiding this comment.
Guard initial API key load so UI init does not abort.
If window.electronAPI.getApiKey() rejects, the async startup handler exits before the settings listeners are registered. Wrap this block in try/catch and continue with inactive state.
Proposed fix
- const existing = await window.electronAPI.getApiKey();
- if (existing) {
- apiKeyInput.placeholder = KEY_SAVED_PLACEHOLDER;
- keyBadge.textContent = 'active';
- keyBadge.className = 'key-badge active';
- }
+ try {
+ const existing = await window.electronAPI.getApiKey();
+ if (existing) {
+ apiKeyInput.placeholder = KEY_SAVED_PLACEHOLDER;
+ keyBadge.textContent = 'active';
+ keyBadge.className = 'key-badge active';
+ }
+ } catch {
+ keyStatus.textContent = 'Key storage unavailable';
+ keyStatus.className = 'key-status error';
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hub.js` around lines 16 - 21, Wrap the initial API-key fetch
(window.electronAPI.getApiKey()) in a try/catch so a rejection doesn't abort
startup; if the call succeeds set apiKeyInput.placeholder =
KEY_SAVED_PLACEHOLDER and set keyBadge.textContent = 'active' and
keyBadge.className = 'key-badge active', but if it throws or returns falsy set
the UI to the inactive state (e.g., keyBadge.textContent = 'inactive' and
keyBadge.className = 'key-badge inactive' and leave apiKeyInput as editable) and
let execution continue so the settings listeners still get registered; update
the block around the getApiKey call accordingly.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/phases.test.ts`:
- Around line 46-49: The test for getCategoryWeight in src/main/phases.test.ts
is currently asserting values for categories that are explicitly defined as 1,
so it doesn't exercise the default/fallback branch; update the test to either
rename it to reflect that it checks explicit weights or change the assertion to
call getCategoryWeight with an unsupported/unknown category (e.g., pass a
made-up category string or cast an unsupported value) to trigger the default
branch and verify it returns 1; locate the call to getCategoryWeight in the test
and replace or add the unknown-category call to exercise the fallback behavior.
In `@src/main/prompts.test.ts`:
- Around line 67-95: The tests currently only check length/uniqueness and have a
no-op reset; update them to assert the exact wraparound sequence using the
existing modes array and verify stopPromptLoop() resets state: call
cycleOutputMode() N times (N = modes.length) and expect the collected sequence
to strictly equal modes, then call stopPromptLoop() and immediately call
cycleOutputMode() and expect that single result to equal modes[0]; replace the
weak assertions in the 'cycles through all modes eventually' and 'reset' tests
to use these explicit comparisons and reference cycleOutputMode(),
stopPromptLoop(), and modes.
In `@src/main/server.test.ts`:
- Around line 4-18: The test duplicates the isValidSnapshot implementation
instead of exercising the real validator; replace the local copy in
server.test.ts with the real function by importing isValidSnapshot from the
server module (or move the validator into a small exported helper and import
that) so the test calls the actual isValidSnapshot implementation used in
production; update any references in the test to use the imported
isValidSnapshot and remove the inline function to ensure regressions in the real
validator are caught.
In `@src/main/templates.test.ts`:
- Around line 82-85: The test passes numeric values to resolveTemplate but the
function signature only accepts Record<string, string>; update resolveTemplate
to accept Record<string, string | number> (or a type alias like TemplateData =
Record<string, string | number>) and ensure resolveTemplate converts values to
strings when interpolating (e.g., String(value)); then run/adjust other call
sites that pass data to resolveTemplate to satisfy the new union type.
Alternatively, if you prefer the smaller change, update the test data in the
numeric-values test to use string values ('6' and '45') instead of numbers so it
matches the existing signature.
In `@test-results/.last-run.json`:
- Around line 1-4: Remove the generated Playwright runner state file
test-results/.last-run.json from the repo and prevent future commits by adding
test-results/ to .gitignore; specifically, delete the .last-run.json file (it
contains keys like "status" and "failedTests") and commit the change, then add a
line excluding test-results/ to .gitignore and commit that so stale last-run
data stops landing in PRs.
In `@tests/electron.spec.ts`:
- Around line 28-31: The test currently validates internal mock state because
triggerSpeakPrompt pushes to speakPrompts before invoking listeners, so update
the test to assert renderer-side behavior (or explicit listener invocation)
rather than the mock array; either (a) change triggerSpeakPrompt in the test
mock so it invokes this.listeners.speakPrompt callbacks first and then pushes to
this.speakPrompts, or (b) keep the mock but add an assertion that the renderer
actually registered a listener (e.g., spy/mock for listeners.speakPrompt
registration) and that the registered callback was invoked after calling
triggerSpeakPrompt; reference the triggerSpeakPrompt function, the speakPrompts
array, and listeners.speakPrompt when making these changes.
In `@tests/renderer.spec.ts`:
- Around line 35-37: The test "handles invalid state gracefully" currently only
asserts the initial idle markup and never exercises the fallback; update the
test to trigger the component's onStateChange handler via the mock (invoke the
mocked onStateChange with an invalid state value) so the component executes its
fallback logic, then assert the coach locator (page.locator('#coach')) still has
the expected idle class; locate the test named "handles invalid state
gracefully" and the mock that exposes onStateChange, call that mock with an
invalid state, wait for UI update, and then perform the
expect(coach).toHaveClass(/idle/) assertion.
- Around line 9-18: The test navigates to the renderer before defining the
required preload API, so window.electronAPI.onStateChange in renderer.js can
throw silently; modify test.beforeEach to inject a mock via page.addInitScript
that defines window.electronAPI with a noop onStateChange (and any other
expected methods) before calling page.goto, and also attach a
page.on('pageerror', ...) handler that fails the test on any page error so
renderer crashes are surfaced; update references in the test to use the mocked
API and assert no page errors occur.
In `@vitest.config.ts`:
- Around line 1-6: The custom test.exclude in defineConfig is overwriting
Vitest's default excludes; import configDefaults from 'vitest/config' and set
test.exclude to [...configDefaults.exclude, 'tests/**'] (keep the existing
defineConfig export and test object) so the new 'tests/**' entry is appended
rather than replacing the built-in excludes.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f1a3f86-86b0-4811-8b96-acc7b1f2d8d3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.prettierrcpackage.jsonplaywright-report/index.htmlplaywright.config.tssrc/main/detectors.test.tssrc/main/phases.test.tssrc/main/prompts.test.tssrc/main/server.test.tssrc/main/templates.test.tstest-results/.last-run.jsontests/electron.spec.tstests/renderer.spec.tsvitest.config.ts
| it('defaults unknown categories to 1', () => { | ||
| expect(getCategoryWeight('early_laning', 'map_awareness')).toBe(1); | ||
| expect(getCategoryWeight('early_laning', 'mental')).toBe(1); | ||
| }); |
There was a problem hiding this comment.
This case does not cover the default branch.
map_awareness and mental are explicit 1 weights in src/main/phases.ts, so Lines 46-49 only verify hard-coded values. Rename the test to match that, or force the fallback path with a casted unsupported category.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/phases.test.ts` around lines 46 - 49, The test for getCategoryWeight
in src/main/phases.test.ts is currently asserting values for categories that are
explicitly defined as 1, so it doesn't exercise the default/fallback branch;
update the test to either rename it to reflect that it checks explicit weights
or change the assertion to call getCategoryWeight with an unsupported/unknown
category (e.g., pass a made-up category string or cast an unsupported value) to
trigger the default branch and verify it returns 1; locate the call to
getCategoryWeight in the test and replace or add the unknown-category call to
exercise the fallback behavior.
| const modes: OutputMode[] = ['overlay', 'speech', 'both']; | ||
|
|
||
| beforeEach(() => { | ||
| stopPromptLoop(); | ||
| }); | ||
|
|
||
| it('cycles through all modes eventually', () => { | ||
| const results: OutputMode[] = []; | ||
| for (let i = 0; i < 6; i++) { | ||
| results.push(cycleOutputMode()); | ||
| } | ||
| expect(results.length).toBe(6); | ||
| }); | ||
|
|
||
| it('cycles to the next mode in sequence', () => { | ||
| const first = cycleOutputMode(); | ||
| const second = cycleOutputMode(); | ||
| const third = cycleOutputMode(); | ||
|
|
||
| const uniqueResults = new Set([first, second, third]); | ||
| expect(uniqueResults.size).toBe(3); | ||
| }); | ||
| }); | ||
|
|
||
| describe('stopPromptLoop', () => { | ||
| it('resets the prompt loop state', () => { | ||
| stopPromptLoop(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These tests do not verify the cycle/reset contract.
Lines 73-88 only check length and uniqueness, so the mode order can change and the suite still passes. Lines 92-94 have no assertion at all. Assert the wraparound sequence against the existing modes array and verify that stopPromptLoop() makes the next cycleOutputMode() match the first call from a fresh reset.
Proposed fix
describe('cycleOutputMode', () => {
const modes: OutputMode[] = ['overlay', 'speech', 'both'];
beforeEach(() => {
stopPromptLoop();
});
it('cycles through all modes eventually', () => {
- const results: OutputMode[] = [];
- for (let i = 0; i < 6; i++) {
- results.push(cycleOutputMode());
- }
- expect(results.length).toBe(6);
+ const first = cycleOutputMode();
+ const second = cycleOutputMode();
+ const third = cycleOutputMode();
+ const fourth = cycleOutputMode();
+
+ expect(new Set([first, second, third])).toEqual(new Set(modes));
+ expect(fourth).toBe(first);
});
it('cycles to the next mode in sequence', () => {
const first = cycleOutputMode();
const second = cycleOutputMode();
const third = cycleOutputMode();
- const uniqueResults = new Set([first, second, third]);
- expect(uniqueResults.size).toBe(3);
+ const firstIndex = modes.indexOf(first);
+ expect(second).toBe(modes[(firstIndex + 1) % modes.length]);
+ expect(third).toBe(modes[(firstIndex + 2) % modes.length]);
});
});
describe('stopPromptLoop', () => {
it('resets the prompt loop state', () => {
- stopPromptLoop();
+ const first = cycleOutputMode();
+ cycleOutputMode();
+ cycleOutputMode();
+ stopPromptLoop();
+ expect(cycleOutputMode()).toBe(first);
});
});📝 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.
| const modes: OutputMode[] = ['overlay', 'speech', 'both']; | |
| beforeEach(() => { | |
| stopPromptLoop(); | |
| }); | |
| it('cycles through all modes eventually', () => { | |
| const results: OutputMode[] = []; | |
| for (let i = 0; i < 6; i++) { | |
| results.push(cycleOutputMode()); | |
| } | |
| expect(results.length).toBe(6); | |
| }); | |
| it('cycles to the next mode in sequence', () => { | |
| const first = cycleOutputMode(); | |
| const second = cycleOutputMode(); | |
| const third = cycleOutputMode(); | |
| const uniqueResults = new Set([first, second, third]); | |
| expect(uniqueResults.size).toBe(3); | |
| }); | |
| }); | |
| describe('stopPromptLoop', () => { | |
| it('resets the prompt loop state', () => { | |
| stopPromptLoop(); | |
| }); | |
| }); | |
| const modes: OutputMode[] = ['overlay', 'speech', 'both']; | |
| beforeEach(() => { | |
| stopPromptLoop(); | |
| }); | |
| it('cycles through all modes eventually', () => { | |
| const first = cycleOutputMode(); | |
| const second = cycleOutputMode(); | |
| const third = cycleOutputMode(); | |
| const fourth = cycleOutputMode(); | |
| expect(new Set([first, second, third])).toEqual(new Set(modes)); | |
| expect(fourth).toBe(first); | |
| }); | |
| it('cycles to the next mode in sequence', () => { | |
| const first = cycleOutputMode(); | |
| const second = cycleOutputMode(); | |
| const third = cycleOutputMode(); | |
| const firstIndex = modes.indexOf(first); | |
| expect(second).toBe(modes[(firstIndex + 1) % modes.length]); | |
| expect(third).toBe(modes[(firstIndex + 2) % modes.length]); | |
| }); | |
| }); | |
| describe('stopPromptLoop', () => { | |
| it('resets the prompt loop state', () => { | |
| const first = cycleOutputMode(); | |
| cycleOutputMode(); | |
| cycleOutputMode(); | |
| stopPromptLoop(); | |
| expect(cycleOutputMode()).toBe(first); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/prompts.test.ts` around lines 67 - 95, The tests currently only
check length/uniqueness and have a no-op reset; update them to assert the exact
wraparound sequence using the existing modes array and verify stopPromptLoop()
resets state: call cycleOutputMode() N times (N = modes.length) and expect the
collected sequence to strictly equal modes, then call stopPromptLoop() and
immediately call cycleOutputMode() and expect that single result to equal
modes[0]; replace the weak assertions in the 'cycles through all modes
eventually' and 'reset' tests to use these explicit comparisons and reference
cycleOutputMode(), stopPromptLoop(), and modes.
| describe('isValidSnapshot (via server module)', () => { | ||
| function isValidSnapshot(data: unknown): data is GameSnapshot { | ||
| if (typeof data !== 'object' || data === null) return false; | ||
| const d = data as Record<string, unknown>; | ||
| return ( | ||
| typeof d.activePlayer === 'object' && | ||
| d.activePlayer !== null && | ||
| Array.isArray(d.allPlayers) && | ||
| typeof d.events === 'object' && | ||
| d.events !== null && | ||
| typeof d.gameData === 'object' && | ||
| d.gameData !== null && | ||
| typeof (d.gameData as Record<string, unknown>).gameTime === 'number' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Test the real validator, not a copy of it.
Lines 5-18 duplicate isValidSnapshot from src/main/server.ts, so this suite still passes if the production check regresses. Import the real validator, or extract it to a small helper and test that instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/server.test.ts` around lines 4 - 18, The test duplicates the
isValidSnapshot implementation instead of exercising the real validator; replace
the local copy in server.test.ts with the real function by importing
isValidSnapshot from the server module (or move the validator into a small
exported helper and import that) so the test calls the actual isValidSnapshot
implementation used in production; update any references in the test to use the
imported isValidSnapshot and remove the inline function to ensure regressions in
the real validator are caught.
| it('handles numeric values in data', () => { | ||
| const template = 'Level {level}, CS {cs}'; | ||
| const data = { level: 6, cs: 45 }; | ||
| expect(resolveTemplate(template, data)).toBe('Level 6, CS 45'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify resolveTemplate/canResolve type contracts:"
fd -i 'templates\.ts$' --exec sed -n '1,120p' {}
echo
echo "Locate numeric test input passed to resolveTemplate:"
rg -n "handles numeric values in data|const data = \{ level: 6, cs: 45 \}|resolveTemplate\(template, data\)" src/main/templates.test.tsRepository: Ilia01/Beacon
Length of output: 1433
Fix the type mismatch in the numeric-values test.
Line 84 passes numeric values { level: 6, cs: 45 }, but resolveTemplate expects Record<string, string>. Either convert values to strings or update the function signature to accept Record<string, string | number> (and check all call sites).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/templates.test.ts` around lines 82 - 85, The test passes numeric
values to resolveTemplate but the function signature only accepts Record<string,
string>; update resolveTemplate to accept Record<string, string | number> (or a
type alias like TemplateData = Record<string, string | number>) and ensure
resolveTemplate converts values to strings when interpolating (e.g.,
String(value)); then run/adjust other call sites that pass data to
resolveTemplate to satisfy the new union type. Alternatively, if you prefer the
smaller change, update the test data in the numeric-values test to use string
values ('6' and '45') instead of numbers so it matches the existing signature.
| { | ||
| "status": "passed", | ||
| "failedTests": [] | ||
| } No newline at end of file |
There was a problem hiding this comment.
Remove generated Playwright state from version control.
test-results/.last-run.json is runner state, not source. Delete this file and ignore test-results/ so stale last-run data does not keep landing in PRs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-results/.last-run.json` around lines 1 - 4, Remove the generated
Playwright runner state file test-results/.last-run.json from the repo and
prevent future commits by adding test-results/ to .gitignore; specifically,
delete the .last-run.json file (it contains keys like "status" and
"failedTests") and commit the change, then add a line excluding test-results/ to
.gitignore and commit that so stale last-run data stops landing in PRs.
| triggerSpeakPrompt: function(text) { | ||
| this.speakPrompts.push(text); | ||
| this.listeners.speakPrompt.forEach(function(cb) { cb(text); }); | ||
| } |
There was a problem hiding this comment.
The speak-prompt test validates mock internals instead of renderer behavior.
triggerSpeakPrompt appends to speakPrompts before invoking listeners, so this assertion passes even if the renderer never subscribed. Assert a renderer-side effect (or at least listener registration/invocation) after triggering the event.
Also applies to: 189-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/electron.spec.ts` around lines 28 - 31, The test currently validates
internal mock state because triggerSpeakPrompt pushes to speakPrompts before
invoking listeners, so update the test to assert renderer-side behavior (or
explicit listener invocation) rather than the mock array; either (a) change
triggerSpeakPrompt in the test mock so it invokes this.listeners.speakPrompt
callbacks first and then pushes to this.speakPrompts, or (b) keep the mock but
add an assertion that the renderer actually registered a listener (e.g.,
spy/mock for listeners.speakPrompt registration) and that the registered
callback was invoked after calling triggerSpeakPrompt; reference the
triggerSpeakPrompt function, the speakPrompts array, and listeners.speakPrompt
when making these changes.
| test.beforeEach(async ({ page }) => { | ||
| const htmlPath = path.join( | ||
| __dirname, | ||
| '..', | ||
| 'src', | ||
| 'renderer', | ||
| 'index.html', | ||
| ); | ||
| await page.goto(`file://${htmlPath}`); | ||
| }); |
There was a problem hiding this comment.
Mock window.electronAPI before navigation and fail on page errors.
src/renderer/renderer.js calls window.electronAPI.onStateChange on load. This suite navigates without defining that object, so renderer crashes can be hidden while static element checks still pass. Add page.addInitScript(...) with the required API shape and assert no pageerror events.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/renderer.spec.ts` around lines 9 - 18, The test navigates to the
renderer before defining the required preload API, so
window.electronAPI.onStateChange in renderer.js can throw silently; modify
test.beforeEach to inject a mock via page.addInitScript that defines
window.electronAPI with a noop onStateChange (and any other expected methods)
before calling page.goto, and also attach a page.on('pageerror', ...) handler
that fails the test on any page error so renderer crashes are surfaced; update
references in the test to use the mocked API and assert no page errors occur.
| test('handles invalid state gracefully', async ({ page }) => { | ||
| const coach = page.locator('#coach'); | ||
| await expect(coach).toHaveClass(/idle/); |
There was a problem hiding this comment.
The invalid-state test never triggers an invalid state.
This assertion only checks the initial idle class from markup; it does not execute fallback logic for an invalid state. Trigger onStateChange with an invalid value through the mock, then assert idle.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/renderer.spec.ts` around lines 35 - 37, The test "handles invalid state
gracefully" currently only asserts the initial idle markup and never exercises
the fallback; update the test to trigger the component's onStateChange handler
via the mock (invoke the mocked onStateChange with an invalid state value) so
the component executes its fallback logic, then assert the coach locator
(page.locator('#coach')) still has the expected idle class; locate the test
named "handles invalid state gracefully" and the mock that exposes
onStateChange, call that mock with an invalid state, wait for UI update, and
then perform the expect(coach).toHaveClass(/idle/) assertion.
| import { defineConfig } from 'vitest/config'; | ||
|
|
||
| export default defineConfig({ | ||
| test: { | ||
| exclude: ['tests/**', '**/node_modules/**'], | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Vitest 4.1.x, does setting test.excludereplace the default exclude list, and isexclude: [...configDefaults.exclude, 'tests/**'] the recommended way to keep the defaults while excluding Playwright tests?
💡 Result:
Yes, in Vitest 4.1.x, setting test.exclude in the config replaces the default exclude list. The official documentation explicitly states: "manually setting exclude in your config will replace the default value." The recommended way to keep the defaults while adding exclusions (such as 'tests/' for Playwright tests) is to use exclude: [...configDefaults.exclude, 'tests/'], as shown in the official docs example: import { configDefaults, defineConfig } from 'vitest/config'; export default defineConfig({ test: { exclude: [ ...configDefaults.exclude, 'packages/template/*', './temp/', ], }, }); This behavior and recommendation are unchanged in Vitest 4.1.x, as confirmed by the v4.1.0 changelog and docs.
Citations:
- 1: https://v4.vitest.dev/config/exclude
- 2: https://vitest.dev/config/exclude
- 3: https://vitest.dev/config/
- 4: https://github.com/vitest-dev/vitest/blob/1a4705da/docs/guide/migration.md
Preserve Vitest's default excludes.
The exclude config replaces Vitest's built-in exclude list. Use [...configDefaults.exclude, 'tests/**'] instead.
Fix
-import { defineConfig } from 'vitest/config';
+import { configDefaults, defineConfig } from 'vitest/config';
export default defineConfig({
test: {
- exclude: ['tests/**', '**/node_modules/**'],
+ exclude: [...configDefaults.exclude, 'tests/**'],
},
});📝 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 { defineConfig } from 'vitest/config'; | |
| export default defineConfig({ | |
| test: { | |
| exclude: ['tests/**', '**/node_modules/**'], | |
| }, | |
| import { configDefaults, defineConfig } from 'vitest/config'; | |
| export default defineConfig({ | |
| test: { | |
| exclude: [...configDefaults.exclude, 'tests/**'], | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vitest.config.ts` around lines 1 - 6, The custom test.exclude in defineConfig
is overwriting Vitest's default excludes; import configDefaults from
'vitest/config' and set test.exclude to [...configDefaults.exclude, 'tests/**']
(keep the existing defineConfig export and test object) so the new 'tests/**'
entry is appended rather than replacing the built-in excludes.
Ilia01
force-pushed
the
feat/llm-coaching
branch
from
de3c9cd to
0868281
Compare
|
@copilot A few real blockers before I merge this.
Keep the fixes tight. I don't want a redesign here, just make the current feature safe and mergeable. |
…on, cooldown unwind Agent-Logs-Url: https://github.com/Ilia01/Beacon/sessions/21e2e23d-d763-4e4b-98a3-29de781db7bb Co-authored-by: Ilia01 <155351862+Ilia01@users.noreply.github.com>
All 6 addressed in commit
|
|
@copilot This branch is still behind current main and it would regress merged behavior if I took it as-is. Rebase or update it on top of current main and preserve the already-merged work from #34, #37, #38, and #40. There is also one real UI bug left in this branch: returns early on an empty key, so there is no way to clear a saved key from the settings UI even though main-process clearing exists. Make empty-save explicitly clear the stored key and runtime client state, then I’ll re-review. |
# Conflicts: # src/renderer/hub.html Co-authored-by: Ilia01 <155351862+Ilia01@users.noreply.github.com>
Done in commit Rebase onto main: Merged Empty-key clear bug: Removed the All 81 tests pass; build is clean. |
What changed
Why
Template prompts felt stale and repetitive. LLM rephrasing makes the same advice sound different each time while keeping the coaching logic deterministic and correct -- no hallucinated strategy.
How to test
npm testpasses (56 tests)Checklist