-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: added fix for cursor jump issue #9284
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
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.
Caution
Changes requested ❌
Reviewed everything up to 471d159 in 4 minutes and 4 seconds. Click for details.
- Reviewed
289
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
6
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/components/QueryBuilderV2/QueryV2/QuerySearch/QuerySearch.tsx:1281
- Draft comment:
Avoid inline styles in the element (used here to style the tooltip link). Please move these style declarations (e.g. color: '#1890ff', textDecoration: 'underline', etc.) into an external stylesheet or use design tokens to ensure consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. frontend/src/components/QueryBuilderV2/QueryV2/QuerySearch/QuerySearch.tsx:142
- Draft comment:
Several useEffect hooks disable exhaustive-deps (via eslint-disable-next-line). Please ensure that all necessary dependencies are included or extract functions to avoid potential stale closures. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%
<= threshold50%
The comment is asking the PR author to ensure that all necessary dependencies are included, which violates the rule against asking the author to ensure something. It also suggests extracting functions, which is a valid suggestion, but the overall comment is more about ensuring correctness rather than providing a specific suggestion or question.
3. frontend/src/providers/QueryBuilder.tsx:424
- Draft comment:
The update code for removing builder entities is duplicated (currentQuery and supersetQuery updates with nearly identical logic). Consider extracting the common update function into a helper to reduce code duplication. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/src/providers/QueryBuilder.tsx:584
- Draft comment:
Found a console.log statement logging 'prevState'. Remove debugging logs before production to avoid cluttering console output. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. frontend/src/providers/QueryBuilder.tsx:396
- Draft comment:
The new handler for setting currentFilterExpression uses the queryName as a key. Please ensure that queryName is always defined and unique across queries so that filter expressions are not inadvertently overwritten. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that a certain condition is met, which violates the rule against asking for confirmation or verification. It does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.
6. frontend/src/pages/LogsExplorer/index.tsx:408
- Draft comment:
Typo: It seems like 'currentyQuery' is a typo. It might be intended to be 'currentQuery'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This appears to be a debug statement that was just added (showing JSON.stringify of some state). While the typo is real, this looks like temporary debug code that was accidentally committed. Debug statements like this are typically removed before merging. The real issue is that the debug statements shouldn't be committed at all. I could be wrong about this being temporary debug code - maybe it's meant to be permanent debug output. Also, even if it is temporary, fixing typos is still good practice. The pattern of using JSON.stringify to dump state variables strongly suggests this is temporary debug code. Even if it stays, a typo in debug output is not important enough to warrant a review comment. The comment should be deleted. The real issue is the committed debug statements, but that's outside the scope of what we're reviewing.
Workflow ID: wflow_3RN3TZmeXyBRWh6e
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed af2f36a in 1 minute and 0 seconds. Click for details.
- Reviewed
24
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/pages/LogsExplorer/index.tsx:90
- Draft comment:
Removed 'currentFilterExpression' from useQueryBuilder destructuring. Ensure it's not needed elsewhere; debug output now removed to prevent unnecessary re-renders. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure that 'currentFilterExpression' is not needed elsewhere, which violates the rule against asking the author to confirm their intention or to double-check things. However, it also provides a reason for the change, which is to prevent unnecessary re-renders, which is informative but not actionable. Overall, the comment is not actionable and violates the rules.
2. frontend/src/pages/LogsExplorer/index.tsx:403
- Draft comment:
Removed debug paragraphs displaying filter expression and query info. For dev diagnostics, consider conditional rendering based on environment. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_LsFHIPsiFOGMBzED
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 5b92697 in 1 minute and 55 seconds. Click for details.
- Reviewed
26
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/container/LogsExplorerViews/tests/mock.ts:64
- Draft comment:
Added 'currentFilterExpression' and 'handleSetCurrentFilterExpression' to the QueryBuilder context mock. Ensure these properties match the updated production interface. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:186
- Draft comment:
Added 'currentFilterExpression' and 'handleSetCurrentFilterExpression' in the test's QueryBuilderContext. Verify that these align with the context changes in production. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_ZobWrwE1OrO1NK2z
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Pull Request Overview
This PR fixes a cursor jump issue in the CodeMirror editor by optimizing query updates. Previously, the query was updated on every keystroke, causing component remounting and object recreation. The solution introduces a separate currentFilterExpression
state that tracks intermediate changes, only updating the main query when the editor loses focus or when running the query.
- Introduces
currentFilterExpression
to track intermediate filter changes separately from main query - Updates query sync logic to only update main query on blur or explicit run
- Adds new context method
handleSetCurrentFilterExpression
for managing filter expressions
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
frontend/src/types/common/queryBuilder.ts | Adds type definitions for new filter expression state and handler |
frontend/src/providers/QueryBuilder.tsx | Implements currentFilterExpression state and handler logic |
frontend/src/pages/LogsExplorer/tests/LogsExplorer.test.tsx | Updates test to include new context properties |
frontend/src/container/LogsExplorerViews/tests/mock.ts | Updates mock to include new context properties |
frontend/src/components/QueryBuilderV2/QueryV2/QueryV2.tsx | Integrates new filter expression handling with syncExpression flag |
frontend/src/components/QueryBuilderV2/QueryV2/QuerySearch/QuerySearch.tsx | Implements editor focus/blur logic and cursor position tracking |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
}): JSX.Element { | ||
const isDarkMode = useIsDarkMode(); | ||
const [query, setQuery] = useState<string>(queryData.filter?.expression || ''); | ||
const [query, setQuery] = useState<string>(''); |
Copilot
AI
Oct 8, 2025
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.
The query state is initialized as empty string instead of the current filter expression. This could cause the editor to show empty content on initial render even when there's an existing filter expression.
const [query, setQuery] = useState<string>(''); | |
const [query, setQuery] = useState<string>(queryData.filterExpression || ''); |
Copilot uses AI. Check for mistakes.
frontend/src/components/QueryBuilderV2/QueryV2/QuerySearch/QuerySearch.tsx
Show resolved
Hide resolved
frontend/src/components/QueryBuilderV2/QueryV2/QuerySearch/QuerySearch.tsx
Show resolved
Hide resolved
4b63a67
to
fafa13f
Compare
Fix: ISSUE:9080
Current Implementation:
Previously, the IbuilderQuery was updated on every keystroke in the CodeMirror editor. This approach led to unnecessary remounting of components and creation of new objects on each key stroke, as the currentQuery was updated every time.
Solution:
The new implementation keeps a different object for storing currentFilterExpression and updates the main currentQuery only either when the editor is unfocused or when running the query . This improves performance by reducing unnecessary updates and operations with every keystroke.
Before:
Screen.Recording.2025-09-20.at.9.00.07.PM.mov
After:
Screen.Recording.2025-09-20.at.9.03.33.PM.mov
Important
Improves query builder performance by updating main query only on blur or run, using
currentFilterExpression
for intermediate changes.QuerySearch.tsx
: UpdatesonChange
to acceptsyncExpression
flag, updates query only on blur or run.QueryV2.tsx
: UseshandleSetCurrentFilterExpression
to manage query state, updates query only whensyncExpression
is true.QueryBuilder.tsx
: IntroducescurrentFilterExpression
to track changes, updates main query on run.handleSetCurrentFilterExpression
toQueryBuilderContext
to manage filter expressions separately from main query.mock.ts
andLogsExplorer.test.tsx
to usecurrentFilterExpression
and new context methods.This description was created by
for 5b92697. You can customize this summary. It will automatically update as commits are pushed.