-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix: useHistory currentIndex more than MAX_HISTORY_LENGTH cause replace fail #8740
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
base: main
Are you sure you want to change the base?
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
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.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="gui/src/hooks/test/useInputHistory.test.ts">
<violation number="1" location="gui/src/hooks/test/useInputHistory.test.ts:305">
`nextRef.current()` returns the pending input when you move forward from the last stored history entry, but this test asserts it is `undefined`, so it will fail whenever the hook returns the stored pending input. Update the expectation to match the hook's behavior.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| act(() => { | ||
| result.current.prevRef.current(emptyJsonContent()); | ||
| const content = result.current.prevRef.current(emptyJsonContent()); | ||
| expect(content).toBeUndefined(); // Should not crash |
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.
nextRef.current() returns the pending input when you move forward from the last stored history entry, but this test asserts it is undefined, so it will fail whenever the hook returns the stored pending input. Update the expectation to match the hook's behavior.
Prompt for AI agents
Address the following comment on gui/src/hooks/test/useInputHistory.test.ts at line 305:
<comment>`nextRef.current()` returns the pending input when you move forward from the last stored history entry, but this test asserts it is `undefined`, so it will fail whenever the hook returns the stored pending input. Update the expectation to match the hook's behavior.</comment>
<file context>
@@ -0,0 +1,370 @@
+ act(() => {
+ result.current.prevRef.current(emptyJsonContent());
+ const content = result.current.prevRef.current(emptyJsonContent());
+ expect(content).toBeUndefined(); // Should not crash
+ });
+
</file context>
| expect(content).toBeUndefined(); // Should not crash | |
| expect(content).toEqual(emptyJsonContent()); |
Description
When add input history more than MAX_HISTORY_LENGTH, cause
pre()fail, becausecurrentIndex = MAX_HISTORY_LENGTH + 1:AI Code Review
@continue-reviewChecklist
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
add
useInputHistory.test.tsto test this cause:History eviction (MAX_HISTORY_LENGTH)Summary by cubic
Fixes input history navigation when history exceeds MAX_HISTORY_LENGTH. Prevents prev/next replace failures by clamping currentIndex.
Written for commit ef260b9. Summary will update automatically on new commits.