fix(#85): Ctrl+R session refresh via IPC with scroll-to-bottom#86
fix(#85): Ctrl+R session refresh via IPC with scroll-to-bottom#86Psypeal wants to merge 8 commits intomatt1398:mainfrom
Conversation
# Conflicts: # src/renderer/components/chat/ContextBadge.tsx # src/renderer/utils/contextTracker.ts
# Conflicts: # src/renderer/components/chat/ContextBadge.tsx
sessionAnalyzer now reads pre-computed costUsd from calculateMetrics() instead of re-computing costs independently. Parent cost uses detail.metrics.costUsd, subagent cost uses proc.metrics.costUsd. This ensures the cost analysis panel and chat header always agree. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Electron's before-input-event preventDefault blocks both Chromium's built-in reload AND keydown propagation to the renderer. Route Ctrl+R through IPC instead: main process intercepts and sends session:refresh, preload bridges to renderer, store listener refreshes in place. Switch all refresh paths (IPC, refresh button, keyboard shortcut) to refreshSessionInPlace to avoid unmounting the scroll container, then dispatch a custom event to smoothly scroll to the bottom. Fixes matt1398#85
Summary of ChangesHello @Psypeal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for session refreshing within the Electron application. It addresses issues with keyboard shortcuts by routing refresh commands through IPC, ensuring that the application's custom refresh logic is always triggered. Additionally, it unifies refresh paths and introduces an automatic scroll-to-bottom feature, making session refreshes smoother and more predictable. A refactor of the session cost calculation also improves data consistency. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully implements the Ctrl+R session refresh feature by routing the shortcut through IPC to the renderer. It also unifies refresh paths to use refreshSessionInPlace and adds a smooth scroll-to-bottom after refresh. The changes are well-implemented and include necessary updates to the preload script, renderer API, and various components. I've included a couple of suggestions to improve maintainability by refactoring some logic. Additionally, the unrelated but beneficial refactoring of cost calculation in sessionAnalyzer.ts to use a single source of truth is a good improvement.
| if ((input.control || input.meta) && !input.shift && input.key.toLowerCase() === 'r') { | ||
| event.preventDefault(); | ||
| mainWindow.webContents.send('session:refresh'); | ||
| return; | ||
| } | ||
| // Also block Ctrl+Shift+R (hard reload) | ||
| if ((input.control || input.meta) && input.shift && input.key.toLowerCase() === 'r') { | ||
| event.preventDefault(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The logic for handling Ctrl+R and Ctrl+Shift+R can be combined to be more concise and reduce duplication of the condition check. This would improve readability and maintainability.
if ((input.control || input.meta) && input.key.toLowerCase() === 'r') {
event.preventDefault();
if (!input.shift) {
mainWindow.webContents.send('session:refresh');
}
return;
}| void Promise.all([ | ||
| state.refreshSessionInPlace(activeTab.projectId, activeTab.sessionId), | ||
| state.fetchSessions(activeTab.projectId), | ||
| ]).then(() => { | ||
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | ||
| }); |
There was a problem hiding this comment.
This logic for refreshing a session and then dispatching a scroll event is duplicated in a few places. It's also present in src/renderer/components/layout/TabBar.tsx and src/renderer/hooks/useKeyboardShortcuts.ts. To improve maintainability and reduce duplication, consider extracting this logic into a new action in one of your Zustand slices (e.g., sessionSlice or sessionDetailSlice). For example, you could create a refreshActiveSessionAndScroll() action.
📝 WalkthroughWalkthroughThe pull request implements an IPC-driven session refresh flow for Ctrl+R / Cmd+R hotkeys. The main process intercepts the hotkey, prevents Chromium's default reload behavior, and sends a session:refresh IPC message to the renderer. The renderer listens for this event and refreshes the active session. Additionally, syntax highlighting for code blocks is enhanced with per-line highlighting support for multiple languages including R, Ruby, PHP, and SQL. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/useKeyboardShortcuts.ts`:
- Around line 263-268: The Promise.all call that runs
refreshSessionInPlace(selectedProjectId, selectedSessionId) and
fetchSessions(selectedProjectId) must have a rejection handler to avoid
unhandled promise rejections; update the Promise.all(...).then(...) chain (the
block that dispatches the 'session-refresh-scroll-bottom' CustomEvent) to append
a .catch(...) that handles/logs errors (e.g., using console.error or your app
logger) and ensures any necessary cleanup, so failures in refreshSessionInPlace
or fetchSessions are handled explicitly.
In `@test/renderer/utils/sessionAnalyzer.test.ts`:
- Line 1560: Replace exact equality assertions for computed decimal totals with
approximate equality checks: change
expect(report.costAnalysis.subagentCostUsd).toBe(0.25) and the similar assertion
for totalSessionCostUsd to use toBeCloseTo, e.g.
expect(report.costAnalysis.subagentCostUsd).toBeCloseTo(0.25, 2) and
expect(report.costAnalysis.totalSessionCostUsd).toBeCloseTo(<expected>, 2), so
floating-point rounding won't cause flaky failures; update the test in
sessionAnalyzer.test.ts where these properties are asserted.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/index.tssrc/preload/constants/ipcChannels.tssrc/preload/index.tssrc/renderer/api/httpClient.tssrc/renderer/components/chat/ChatHistory.tsxsrc/renderer/components/layout/TabBar.tsxsrc/renderer/hooks/useKeyboardShortcuts.tssrc/renderer/store/index.tssrc/renderer/utils/sessionAnalyzer.tssrc/shared/types/api.tstest/renderer/utils/sessionAnalyzer.test.ts
| void Promise.all([ | ||
| fetchSessionDetail(selectedProjectId, selectedSessionId), | ||
| refreshSessionInPlace(selectedProjectId, selectedSessionId), | ||
| fetchSessions(selectedProjectId), | ||
| ]); | ||
| ]).then(() => { | ||
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/renderer/hooks/useKeyboardShortcuts.ts | sed -n '250,280p'Repository: matt1398/claude-devtools
Length of output: 1207
🏁 Script executed:
# Get context about the imports and Zustand store usage
head -50 src/renderer/hooks/useKeyboardShortcuts.tsRepository: matt1398/claude-devtools
Length of output: 1370
🏁 Script executed:
# Check if there's error handling in the Zustand store being used
rg -A 5 -B 5 "useSessionStore\|useProjectStore" src/renderer/hooks/useKeyboardShortcuts.ts | head -100Repository: matt1398/claude-devtools
Length of output: 50
🏁 Script executed:
# Find the store file
find src -name "store*" -type f | head -20Repository: matt1398/claude-devtools
Length of output: 50
🏁 Script executed:
# Check the actual store implementation to understand error handling
head -100 src/renderer/store/index.tsRepository: matt1398/claude-devtools
Length of output: 4342
🏁 Script executed:
# Look for how other parts of the renderer handle errors with the store
rg "catch\|setError\|error" src/renderer/store --type ts --type tsx -A 2 -B 2 | head -100Repository: matt1398/claude-devtools
Length of output: 95
🏁 Script executed:
# Search for error handling patterns in the codebase, checking for notification or error state
rg "error|Error|catch" src/renderer/store/slices --type ts -A 2 -B 2 | head -150Repository: matt1398/claude-devtools
Length of output: 9676
🏁 Script executed:
# Look at the sessionSlice to see how refreshSessionInPlace is implemented
cat src/renderer/store/slices/sessionSlice.ts | head -200Repository: matt1398/claude-devtools
Length of output: 7181
🏁 Script executed:
# Check if there's a notificationSlice and how it handles errors
cat src/renderer/store/slices/notificationSlice.ts | head -100Repository: matt1398/claude-devtools
Length of output: 3415
🏁 Script executed:
# Find refreshSessionInPlace implementation in the store
rg -A 20 "refreshSessionInPlace" src/renderer/store/slices/sessionSlice.ts | head -60Repository: matt1398/claude-devtools
Length of output: 50
🏁 Script executed:
# Also check sessionDetailSlice since it might have the implementation
rg -A 20 "refreshSessionInPlace" src/renderer/store/slices/sessionDetailSlice.ts | head -60Repository: matt1398/claude-devtools
Length of output: 2156
Add rejection handler to prevent unhandled promise rejections.
The Promise.all(...).then(...) chain at line 263 lacks a .catch() handler. While the underlying functions (refreshSessionInPlace and fetchSessions) have internal error handling, the promise chain itself can still surface as an unhandled rejection without explicit rejection handling.
🔧 Suggested fix
if (selectedProjectId && selectedSessionId) {
void Promise.all([
refreshSessionInPlace(selectedProjectId, selectedSessionId),
fetchSessions(selectedProjectId),
- ]).then(() => {
- window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom'));
- });
+ ])
+ .then(() => {
+ window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom'));
+ })
+ .catch((error) => {
+ console.error('Failed to refresh session via keyboard shortcut', error);
+ });
}📝 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.
| void Promise.all([ | |
| fetchSessionDetail(selectedProjectId, selectedSessionId), | |
| refreshSessionInPlace(selectedProjectId, selectedSessionId), | |
| fetchSessions(selectedProjectId), | |
| ]); | |
| ]).then(() => { | |
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | |
| }); | |
| void Promise.all([ | |
| refreshSessionInPlace(selectedProjectId, selectedSessionId), | |
| fetchSessions(selectedProjectId), | |
| ]) | |
| .then(() => { | |
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to refresh session via keyboard shortcut', error); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/useKeyboardShortcuts.ts` around lines 263 - 268, The
Promise.all call that runs refreshSessionInPlace(selectedProjectId,
selectedSessionId) and fetchSessions(selectedProjectId) must have a rejection
handler to avoid unhandled promise rejections; update the
Promise.all(...).then(...) chain (the block that dispatches the
'session-refresh-scroll-bottom' CustomEvent) to append a .catch(...) that
handles/logs errors (e.g., using console.error or your app logger) and ensures
any necessary cleanup, so failures in refreshSessionInPlace or fetchSessions are
handled explicitly.
| const report = analyzeSession(createMockDetail({ processes })); | ||
| expect(report.subagentMetrics.byAgent[0].costUsd).toBe(0.15); | ||
| expect(report.subagentMetrics.byAgent[1].costUsd).toBe(0.10); | ||
| expect(report.costAnalysis.subagentCostUsd).toBe(0.25); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
wc -l test/renderer/utils/sessionAnalyzer.test.tsRepository: matt1398/claude-devtools
Length of output: 116
🏁 Script executed:
# Read the specific lines mentioned in the review comment (around lines 1560 and 1587)
sed -n '1550,1600p' test/renderer/utils/sessionAnalyzer.test.ts | cat -nRepository: matt1398/claude-devtools
Length of output: 1732
Use toBeCloseTo for computed decimal totals.
Assertions for subagentCostUsd and totalSessionCostUsd are computed sums and should use toBeCloseTo() instead of toBe() to be resilient to floating-point precision issues.
🔧 Suggested fix
- expect(report.costAnalysis.subagentCostUsd).toBe(0.25);
+ expect(report.costAnalysis.subagentCostUsd).toBeCloseTo(0.25, 8);
@@
- expect(report.costAnalysis.totalSessionCostUsd).toBe(0.80);
+ expect(report.costAnalysis.totalSessionCostUsd).toBeCloseTo(0.80, 8);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/renderer/utils/sessionAnalyzer.test.ts` at line 1560, Replace exact
equality assertions for computed decimal totals with approximate equality
checks: change expect(report.costAnalysis.subagentCostUsd).toBe(0.25) and the
similar assertion for totalSessionCostUsd to use toBeCloseTo, e.g.
expect(report.costAnalysis.subagentCostUsd).toBeCloseTo(0.25, 2) and
expect(report.costAnalysis.totalSessionCostUsd).toBeCloseTo(<expected>, 2), so
floating-point rounding won't cause flaky failures; update the test in
sessionAnalyzer.test.ts where these properties are asserted.
Add keyword sets and comment detection for four new languages in the token-based syntax highlighter. Wire highlightLine into MarkdownViewer so fenced code blocks in assistant text also receive coloring. - R: keywords, # comments - Ruby: keywords, # comments - PHP: keywords, # comments - SQL: keywords: (case-insensitive), -- comments
# Conflicts: # src/renderer/utils/sessionAnalyzer.ts # test/renderer/utils/sessionAnalyzer.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/renderer/components/chat/viewers/syntaxHighlighter.ts (1)
236-279:block_given?will never match due to word regex pattern.The word match regex on line 535 (
/^([a-zA-Z_$][a-zA-Z0-9_$]*)/) does not include?, soblock_given?will be tokenized asblock_given(identifier) followed by?(operator). Consider removing it from the keyword set or adjusting the regex to handle Ruby's method naming conventions.Option 1: Remove the unmatchable keyword
'yield', - 'block_given?', 'require',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/chat/viewers/syntaxHighlighter.ts` around lines 236 - 279, The keyword "block_given?" in the ruby Set will never match because the identifier regex used in the tokenizer (/^([a-zA-Z_$][a-zA-Z0-9_$]*)/) does not allow a trailing '?'—either remove "block_given?" from the ruby keywords Set or update the tokenizer's identifier regex (used where the word match is performed) to accept Ruby method name suffixes like '?' (e.g., allow an optional trailing '?' in the pattern) so "block_given?" is recognized as a single keyword/token; update references to the ruby Set and the tokenizer function that uses the word-regex (the identifier matching logic) accordingly.src/renderer/components/chat/viewers/MarkdownViewer.tsx (1)
158-171: Consider handling non-string children gracefully.If
childrenis already a React element (not a string),rawwill be empty andlineswill contain a single empty string, resulting in no visible code. This could happen if react-markdown pre-processes the content in certain edge cases.Proposed defensive fallback
if (isBlock) { const lang = codeClassName?.replace('language-', '') ?? ''; const raw = typeof children === 'string' ? children : ''; + // Fallback to original children if not a string + if (!raw && children) { + return ( + <code className="font-mono text-xs" style={{ color: COLOR_TEXT }}> + {children} + </code> + ); + } const text = raw.replace(/\n$/, ''); const lines = text.split('\n');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/chat/viewers/MarkdownViewer.tsx` around lines 158 - 171, The current MarkdownViewer code assumes children is a string and turns non-string children into an empty code block; update the render to handle non-string children by short-circuiting: if typeof children !== 'string' (or React.isValidElement(children)), return the <code className="font-mono text-xs" style={{ color: COLOR_TEXT }}>{children}</code> directly instead of running it through raw/lines/hl/highlightLine; otherwise keep the existing logic that computes raw/text/lines and maps lines with hl(highlightLine(...)). This preserves existing highlighting for string input while gracefully rendering pre-processed React nodes.src/renderer/components/layout/TabBar.tsx (1)
215-223: Add error handling for graceful degradation.The async
handleRefreshfunction lacks try/catch. If eitherrefreshSessionInPlaceorfetchSessionsrejects, the error is silently swallowed (since theonClickhandler doesn't await). Users receive no feedback on failure.♻️ Proposed fix to add error handling
const handleRefresh = async (): Promise<void> => { if (activeTab?.type === 'session' && activeTab.projectId && activeTab.sessionId) { + try { await Promise.all([ refreshSessionInPlace(activeTab.projectId, activeTab.sessionId), fetchSessions(activeTab.projectId), ]); window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); + } catch (error) { + console.error('Failed to refresh session:', error); + } } };As per coding guidelines: "Implement error handling with try/catch in main process and console.error for debugging."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/layout/TabBar.tsx` around lines 215 - 223, The handleRefresh function should guard the Promise.all call with a try/catch so failures in refreshSessionInPlace or fetchSessions are handled: wrap the existing await Promise.all([...]) and the subsequent window.dispatchEvent call in a try block and in the catch log the error via console.error (and optionally trigger any UI fallback/error event), ensuring handleRefresh still returns gracefully when an error occurs; update references to handleRefresh, refreshSessionInPlace, fetchSessions, and the session-refresh-scroll-bottom dispatch accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/components/chat/viewers/MarkdownViewer.tsx`:
- Around line 158-171: The current MarkdownViewer code assumes children is a
string and turns non-string children into an empty code block; update the render
to handle non-string children by short-circuiting: if typeof children !==
'string' (or React.isValidElement(children)), return the <code
className="font-mono text-xs" style={{ color: COLOR_TEXT }}>{children}</code>
directly instead of running it through raw/lines/hl/highlightLine; otherwise
keep the existing logic that computes raw/text/lines and maps lines with
hl(highlightLine(...)). This preserves existing highlighting for string input
while gracefully rendering pre-processed React nodes.
In `@src/renderer/components/chat/viewers/syntaxHighlighter.ts`:
- Around line 236-279: The keyword "block_given?" in the ruby Set will never
match because the identifier regex used in the tokenizer
(/^([a-zA-Z_$][a-zA-Z0-9_$]*)/) does not allow a trailing '?'—either remove
"block_given?" from the ruby keywords Set or update the tokenizer's identifier
regex (used where the word match is performed) to accept Ruby method name
suffixes like '?' (e.g., allow an optional trailing '?' in the pattern) so
"block_given?" is recognized as a single keyword/token; update references to the
ruby Set and the tokenizer function that uses the word-regex (the identifier
matching logic) accordingly.
In `@src/renderer/components/layout/TabBar.tsx`:
- Around line 215-223: The handleRefresh function should guard the Promise.all
call with a try/catch so failures in refreshSessionInPlace or fetchSessions are
handled: wrap the existing await Promise.all([...]) and the subsequent
window.dispatchEvent call in a try block and in the catch log the error via
console.error (and optionally trigger any UI fallback/error event), ensuring
handleRefresh still returns gracefully when an error occurs; update references
to handleRefresh, refreshSessionInPlace, fetchSessions, and the
session-refresh-scroll-bottom dispatch accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/renderer/components/chat/ChatHistory.tsxsrc/renderer/components/chat/viewers/MarkdownViewer.tsxsrc/renderer/components/chat/viewers/syntaxHighlighter.tssrc/renderer/components/layout/TabBar.tsxsrc/shared/types/api.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/shared/types/api.ts
- src/renderer/components/chat/ChatHistory.tsx
|
Superseded by #89 — rebased onto a dedicated feature branch to resolve conflicts. |
1 participant
Summary
event.preventDefault()inbefore-input-eventblocks both Chromium reload and keydown propagationrefreshSessionInPlaceto avoid unmounting the scroll containersession-refresh-scroll-bottomcustom event to smoothly scroll to bottom after refreshTest plan
onSessionRefreshFixes #85
Summary by CodeRabbit
Release Notes