fix(#85): Ctrl+R session refresh via IPC with scroll-to-bottom#89
Conversation
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 resolves an issue where the Ctrl+R shortcut for refreshing a session was not functioning as intended due to conflicts with Electron's default behavior. It introduces a robust IPC mechanism to correctly capture and process the refresh command, standardizes the refresh logic across all entry points, and enhances the user experience by automatically scrolling the chat view to the bottom after a refresh. This ensures consistent and smooth session refreshing without visual disruptions. 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
|
📝 WalkthroughWalkthroughThis PR implements keyboard-driven session refresh for both Electron and browser environments by intercepting Ctrl/Cmd+R in the main process, using IPC to communicate with the renderer, and dispatching a custom event to scroll the chat to the bottom after refresh. Changes
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/hooks/useKeyboardShortcuts.ts (1)
260-270:⚠️ Potential issue | 🟡 MinorAdd error handling for the session refresh operation.
Similar to the store listener, this
Promise.alllacks error handling. If either operation fails, the error will be silently swallowed and the scroll event won't fire.🛡️ Proposed fix to add error handling
// Cmd+R: Refresh current session and sidebar session list if (event.key === 'r') { event.preventDefault(); 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:', error); + }); } return; }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/hooks/useKeyboardShortcuts.ts` around lines 260 - 270, The Promise.all call that runs refreshSessionInPlace(selectedProjectId, selectedSessionId) and fetchSessions(selectedProjectId) needs error handling so failures aren't swallowed; wrap the async work in a try/catch (or append a .catch) that logs the error with console.error and prevents silent failures, and only dispatch the 'session-refresh-scroll-bottom' CustomEvent on success. Locate the block using refreshSessionInPlace, fetchSessions and window.dispatchEvent and add the try/catch (or Promise.catch) to log errors and avoid firing the scroll event when an error occurs.
🧹 Nitpick comments (1)
src/main/index.ts (1)
492-496: Consider using a shared constant for the IPC channel name.The channel name
'session:refresh'is hardcoded here whileSESSION_REFRESHis defined insrc/preload/constants/ipcChannels.ts. I see the comment at lines 46-51 explains this avoids a boundary violation between main and preload. However, if the constant value changes in the future, this hardcoded string could diverge.Consider either:
- Moving shared IPC channel constants to
@shared/constants(accessible to both main and preload)- Documenting the coupling in both locations
This is a minor maintainability concern since the current approach is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/index.ts` around lines 492 - 496, The hardcoded IPC channel string in mainWindow.webContents.send('session:refresh') should be replaced with a shared constant to avoid divergence; either move SESSION_REFRESH from src/preload/constants/ipcChannels.ts into a module accessible to both main and preload (e.g., `@shared/constants/ipcChannels`) or create a small shared constant export and import it into src/main/index.ts, then change the send call to use that constant (SESSION_REFRESH) so both sender (main) and receiver (preload) reference the same identifier.
🤖 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/components/layout/TabBar.tsx`:
- Around line 215-223: The handleRefresh function currently awaits
refreshSessionInPlace and fetchSessions with no error handling; wrap the async
work in a try/catch inside handleRefresh, call await Promise.all(...) in the try
block, and in the catch set a component-level error state (e.g., using useState
like [refreshError, setRefreshError]) or call an existing error handler so
failures in refreshSessionInPlace and fetchSessions produce visible renderer
error state; also ensure the catch logs the error and does not suppress the
window.dispatchEvent call when appropriate.
In `@src/renderer/store/index.ts`:
- Around line 271-289: Wrap the session refresh Promise.all invocation in
explicit error handling so rejections are not swallowed: when handling the
api.onSessionRefresh callback (inside the block using useStore.getState and
activeTab), replace the bare void Promise.all([...]).then(...) with an
async/try-catch (or add a .catch) around state.refreshSessionInPlace and
state.fetchSessions so any error is caught; log the error with console.error
including contextual info (e.g., projectId/sessionId and the error) and still
avoid leaving unhandled rejections—ensure
window.dispatchEvent('session-refresh-scroll-bottom') remains in the success
path only.
---
Outside diff comments:
In `@src/renderer/hooks/useKeyboardShortcuts.ts`:
- Around line 260-270: The Promise.all call that runs
refreshSessionInPlace(selectedProjectId, selectedSessionId) and
fetchSessions(selectedProjectId) needs error handling so failures aren't
swallowed; wrap the async work in a try/catch (or append a .catch) that logs the
error with console.error and prevents silent failures, and only dispatch the
'session-refresh-scroll-bottom' CustomEvent on success. Locate the block using
refreshSessionInPlace, fetchSessions and window.dispatchEvent and add the
try/catch (or Promise.catch) to log errors and avoid firing the scroll event
when an error occurs.
---
Nitpick comments:
In `@src/main/index.ts`:
- Around line 492-496: The hardcoded IPC channel string in
mainWindow.webContents.send('session:refresh') should be replaced with a shared
constant to avoid divergence; either move SESSION_REFRESH from
src/preload/constants/ipcChannels.ts into a module accessible to both main and
preload (e.g., `@shared/constants/ipcChannels`) or create a small shared constant
export and import it into src/main/index.ts, then change the send call to use
that constant (SESSION_REFRESH) so both sender (main) and receiver (preload)
reference the same identifier.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
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/shared/types/api.ts
| const handleRefresh = async (): Promise<void> => { | ||
| if (activeTab?.type === 'session' && activeTab.projectId && activeTab.sessionId) { | ||
| await Promise.all([ | ||
| fetchSessionDetail(activeTab.projectId, activeTab.sessionId, activeTabId ?? undefined), | ||
| refreshSessionInPlace(activeTab.projectId, activeTab.sessionId), | ||
| fetchSessions(activeTab.projectId), | ||
| ]); | ||
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add error handling for the async refresh operations.
The handleRefresh function lacks error handling. If refreshSessionInPlace or fetchSessions fails, the error propagates silently with no user feedback. As per coding guidelines, renderer components should use error state for error handling.
🛡️ Proposed fix to add error handling
const handleRefresh = async (): Promise<void> => {
if (activeTab?.type === 'session' && activeTab.projectId && activeTab.sessionId) {
- await Promise.all([
- refreshSessionInPlace(activeTab.projectId, activeTab.sessionId),
- fetchSessions(activeTab.projectId),
- ]);
- window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom'));
+ 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);
+ }
}
};📝 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 handleRefresh = async (): Promise<void> => { | |
| if (activeTab?.type === 'session' && activeTab.projectId && activeTab.sessionId) { | |
| await Promise.all([ | |
| fetchSessionDetail(activeTab.projectId, activeTab.sessionId, activeTabId ?? undefined), | |
| refreshSessionInPlace(activeTab.projectId, activeTab.sessionId), | |
| fetchSessions(activeTab.projectId), | |
| ]); | |
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | |
| } | |
| }; | |
| 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); | |
| } | |
| } | |
| }; |
🤖 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 currently awaits refreshSessionInPlace and fetchSessions
with no error handling; wrap the async work in a try/catch inside handleRefresh,
call await Promise.all(...) in the try block, and in the catch set a
component-level error state (e.g., using useState like [refreshError,
setRefreshError]) or call an existing error handler so failures in
refreshSessionInPlace and fetchSessions produce visible renderer error state;
also ensure the catch logs the error and does not suppress the
window.dispatchEvent call when appropriate.
| // Listen for Ctrl+R / Cmd+R session refresh from main process (fixes #85) | ||
| if (api.onSessionRefresh) { | ||
| const cleanup = api.onSessionRefresh(() => { | ||
| const state = useStore.getState(); | ||
| const activeTabId = state.activeTabId; | ||
| const activeTab = activeTabId ? state.openTabs.find((t) => t.id === activeTabId) : null; | ||
| if (activeTab?.type === 'session' && activeTab.projectId && activeTab.sessionId) { | ||
| void Promise.all([ | ||
| state.refreshSessionInPlace(activeTab.projectId, activeTab.sessionId), | ||
| state.fetchSessions(activeTab.projectId), | ||
| ]).then(() => { | ||
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | ||
| }); | ||
| } | ||
| }); | ||
| if (typeof cleanup === 'function') { | ||
| cleanupFns.push(cleanup); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for the session refresh operation.
The Promise.all call lacks error handling. If either refreshSessionInPlace or fetchSessions rejects, the error will be silently swallowed (unhandled promise rejection), and the scroll event won't be dispatched.
🛡️ Proposed fix to add error handling
if (api.onSessionRefresh) {
const cleanup = api.onSessionRefresh(() => {
const state = useStore.getState();
const activeTabId = state.activeTabId;
const activeTab = activeTabId ? state.openTabs.find((t) => t.id === activeTabId) : null;
if (activeTab?.type === 'session' && activeTab.projectId && activeTab.sessionId) {
void Promise.all([
state.refreshSessionInPlace(activeTab.projectId, activeTab.sessionId),
state.fetchSessions(activeTab.projectId),
- ]).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:', error);
+ });
}
});As per coding guidelines: "Implement error handling with try/catch in main process and console.error for debugging."
📝 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.
| // Listen for Ctrl+R / Cmd+R session refresh from main process (fixes #85) | |
| if (api.onSessionRefresh) { | |
| const cleanup = api.onSessionRefresh(() => { | |
| const state = useStore.getState(); | |
| const activeTabId = state.activeTabId; | |
| const activeTab = activeTabId ? state.openTabs.find((t) => t.id === activeTabId) : null; | |
| if (activeTab?.type === 'session' && activeTab.projectId && activeTab.sessionId) { | |
| void Promise.all([ | |
| state.refreshSessionInPlace(activeTab.projectId, activeTab.sessionId), | |
| state.fetchSessions(activeTab.projectId), | |
| ]).then(() => { | |
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | |
| }); | |
| } | |
| }); | |
| if (typeof cleanup === 'function') { | |
| cleanupFns.push(cleanup); | |
| } | |
| } | |
| // Listen for Ctrl+R / Cmd+R session refresh from main process (fixes `#85`) | |
| if (api.onSessionRefresh) { | |
| const cleanup = api.onSessionRefresh(() => { | |
| const state = useStore.getState(); | |
| const activeTabId = state.activeTabId; | |
| const activeTab = activeTabId ? state.openTabs.find((t) => t.id === activeTabId) : null; | |
| if (activeTab?.type === 'session' && activeTab.projectId && activeTab.sessionId) { | |
| void Promise.all([ | |
| state.refreshSessionInPlace(activeTab.projectId, activeTab.sessionId), | |
| state.fetchSessions(activeTab.projectId), | |
| ]) | |
| .then(() => { | |
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to refresh session:', error); | |
| }); | |
| } | |
| }); | |
| if (typeof cleanup === 'function') { | |
| cleanupFns.push(cleanup); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/store/index.ts` around lines 271 - 289, Wrap the session refresh
Promise.all invocation in explicit error handling so rejections are not
swallowed: when handling the api.onSessionRefresh callback (inside the block
using useStore.getState and activeTab), replace the bare void
Promise.all([...]).then(...) with an async/try-catch (or add a .catch) around
state.refreshSessionInPlace and state.fetchSessions so any error is caught; log
the error with console.error including contextual info (e.g.,
projectId/sessionId and the error) and still avoid leaving unhandled
rejections—ensure window.dispatchEvent('session-refresh-scroll-bottom') remains
in the success path only.
![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 correctly implements session refresh via IPC to handle Ctrl+R, which is a solid improvement. The changes are consistent across the main, preload, and renderer processes, and the use of a custom event to trigger scrolling is a good approach. My review includes a few suggestions to enhance maintainability by reducing code duplication and extracting magic strings into constants.
| 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 if conditions for handling Ctrl+R (with and without Shift) are very similar and can be combined to reduce code duplication and improve readability. You can use a single if to check for Ctrl+R and then an inner if to handle the case without the Shift key.
if ((input.control || input.meta) && input.key.toLowerCase() === 'r') {
event.preventDefault();
if (!input.shift) {
mainWindow.webContents.send('session:refresh');
}
return;
}| window.addEventListener('session-refresh-scroll-bottom', handler); | ||
| return () => window.removeEventListener('session-refresh-scroll-bottom', handler); |
There was a problem hiding this comment.
The event name session-refresh-scroll-bottom is used as a "magic string" in multiple files (ChatHistory.tsx, TabBar.tsx, useKeyboardShortcuts.ts, store/index.ts). To improve maintainability and prevent potential typos, it would be better to define this as a constant in a shared file (e.g., src/renderer/constants/events.ts) and import it where needed.
| const handleRefresh = async (): Promise<void> => { | ||
| if (activeTab?.type === 'session' && activeTab.projectId && activeTab.sessionId) { | ||
| await Promise.all([ | ||
| fetchSessionDetail(activeTab.projectId, activeTab.sessionId, activeTabId ?? undefined), | ||
| refreshSessionInPlace(activeTab.projectId, activeTab.sessionId), | ||
| fetchSessions(activeTab.projectId), | ||
| ]); | ||
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This session refresh logic is duplicated in a few places. Please see my comment on src/renderer/store/index.ts for a suggestion to centralize it in a store action.
Also, the event name session-refresh-scroll-bottom is a magic string. Please see my comment on src/renderer/components/chat/ChatHistory.tsx for a suggestion to define it as a constant.
| if (selectedProjectId && selectedSessionId) { | ||
| 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.
This session refresh logic is duplicated in a few places. Please see my comment on src/renderer/store/index.ts for a suggestion to centralize it in a store action.
Also, the event name session-refresh-scroll-bottom is a magic string. Please see my comment on src/renderer/components/chat/ChatHistory.tsx for a suggestion to define it as a constant.
| if (api.onSessionRefresh) { | ||
| const cleanup = api.onSessionRefresh(() => { | ||
| const state = useStore.getState(); | ||
| const activeTabId = state.activeTabId; | ||
| const activeTab = activeTabId ? state.openTabs.find((t) => t.id === activeTabId) : null; | ||
| if (activeTab?.type === 'session' && activeTab.projectId && activeTab.sessionId) { | ||
| void Promise.all([ | ||
| state.refreshSessionInPlace(activeTab.projectId, activeTab.sessionId), | ||
| state.fetchSessions(activeTab.projectId), | ||
| ]).then(() => { | ||
| window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); | ||
| }); | ||
| } | ||
| }); | ||
| if (typeof cleanup === 'function') { | ||
| cleanupFns.push(cleanup); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for refreshing a session and dispatching the scroll event is duplicated in store/index.ts, TabBar.tsx, and useKeyboardShortcuts.ts. To improve maintainability and ensure consistency, this logic could be extracted into a single action within your Zustand store.
For example, you could create an action like refreshActiveSessionAndScroll in one of your store slices:
// In a relevant store slice
refreshActiveSessionAndScroll: async () => {
const { selectedProjectId, selectedSessionId, refreshSessionInPlace, fetchSessions } = get();
if (selectedProjectId && selectedSessionId) {
await Promise.all([
refreshSessionInPlace(selectedProjectId, selectedSessionId),
fetchSessions(selectedProjectId),
]);
window.dispatchEvent(new CustomEvent('session-refresh-scroll-bottom')); // Consider using a constant for the event name
}
}Then, all three locations could simply call useStore.getState().refreshActiveSessionAndScroll(). This would centralize the refresh logic.
2 participants
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 refreshRebased onto latest main (after #87 revert) from a dedicated feature branch.
Test plan
onSessionRefreshFixes #85
Summary by CodeRabbit