-
Notifications
You must be signed in to change notification settings - Fork 3.6k
chore: remove chat history from redux persistent storage #7944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uinstinct when I tried this earlier it caused failure to load last session, do we need to add loading of last session on startup from JSON?
I see. There is a slight delay on the first load. feat.mp4 |
@uinstinct I mean on startup it won't load the last session like it currently does. The last session button would operate as before |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comments
feat.mp4implemented! |
export function setupStore(options: { ideMessenger?: IIdeMessenger }) { | ||
const ideMessenger = options.ideMessenger ?? new IdeMessenger(); | ||
|
||
const logger = createLogger({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be used if you uncomment the .concat(logger)
below. For debugging
gui/src/redux/thunks/session.ts
Outdated
async (_, { extra, dispatch, getState }) => { | ||
let lastSessionId = getState().session.lastSessionId; | ||
|
||
const lastSessionResult = await extra.ideMessenger.request("history/list", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just used persisted lastSessionId still rather than listing history. If there's noLast sessionId we start a new session
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said might just comment it out since we will want some version of this soon for per-workspace sessions!
gui/src/hooks/ParallelListeners.tsx
Outdated
}, []); | ||
|
||
useEffect(() => { | ||
void dispatch(loadLastSession()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is pretty critical loading could we add some retry logic and error handling? Maybe retry with exponential backoff or a few times or similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to retry for redux dispatch? because loadLastSession has error catching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, retry the loading of the session, since the dispatch won't fail, if loadLastSession catches an error for this initial load (maybe pass a boolean or unwrap dispatch) then retry once or twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uinstinct alternatively we could loadLastSession after initial config load, as a way to detect that core is up and running (will always be with vs code but not necessarily with jetbrains). I.e. remove the use effect and add load last session into the various things triggered on initial config load
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented!
gui/src/hooks/ParallelListeners.tsx
Outdated
}, []); | ||
|
||
useEffect(() => { | ||
void dispatch(loadLastSession()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uinstinct alternatively we could loadLastSession after initial config load, as a way to detect that core is up and running (will always be with vs code but not necessarily with jetbrains). I.e. remove the use effect and add load last session into the various things triggered on initial config load
Description
Remove session reducer's history item from redux's persistent storage which was stored in vscode's session storage (having cases exceeding 5mb limit).
closes CON-3773
AI Code Review
@continue-general-review
or@continue-detailed-review
Checklist
Screen recording or screenshot
[ When applicable, please include a short screen recording or screenshot - this makes it much easier for us as contributors to review and understand your changes. See this PR as a good example. ]
Tests
[ What tests were added or updated to ensure the changes work as expected? ]
Summary by cubic
Removed persisting chat history in Redux to VS Code session storage to avoid hitting the 5 MB limit and prevent errors on large sessions. Closes CON-3773.
Bug Fixes
Refactors