-
Notifications
You must be signed in to change notification settings - Fork 510
7023-cb-find-and-replace-new #4085
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: devel
Are you sure you want to change the base?
Conversation
…o UI-kit add chevron Icon to UI-kit (used in Search Panel)
…ggle functionality
…archPanel components
…ve match index and triggering search on find actions
That functionality comes in handy when we want to use Undo and Replace All functions together: Before that commit we used to perform N operations where N is a number of replaced cells. And history would record them as atomic operations, so pressing Undo would revert only the last replace. setMany enables batch editing and batch reverting, so now we can Replace All and then revert it by using one Undo operation
…in useGridSearch
sergeyteleshev
left a comment
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.
All in all feature looks awesome! Nice integration with undo/redo btw
Please also consider cases with readonly tables (probably block replace functionality with disabled state) - Session Manager, readonly tables, tables where we cannot edit cells, but can add/delete rows
Also, keep in mind that if we replace 1 item. We want to scroll and focus this item because it may be outside the visible view, and we need to show the user that this value has changed. For "replace all" the view should remain the same (no selection, no scroll to cell)
Some of the issues can be resolved in the next tickets. We need to decide what we can do now and what later
| escapedSearch = `\\b${escapedSearch}\\b`; | ||
| } | ||
|
|
||
| const flags = options.caseSensitive ? 'g' : 'gi'; |
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.
duplicated twice. can be moved to the top of function
plus lets move g and gi to consts with name so no more magic numbers/letters
| } catch { | ||
| return null; | ||
| } |
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 is a silent quit. which from the code perspective basically equals to nothing have been found
what if user enables regexp, then tries something that causes an error. as result it sees - nothing have found. but in fact there is an error. maybe we can show the error to the user just to know the search query is screwed and need to try something else. it would improve ux for the users for sure
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 is how the search works in Value Panel right now. If we want to show something, we need to groom it first.
| for (let rowIdx = 0; rowIdx < rowCount; rowIdx++) { | ||
| for (let colIdx = 0; colIdx < columnCount; colIdx++) { | ||
| const cellText = getCellText(rowIdx, colIdx); | ||
| if (searchPattern.test(cellText)) { | ||
| matches.push({ rowIdx, colIdx }); | ||
| if (searchPattern.global) { | ||
| searchPattern.lastIndex = 0; | ||
| } | ||
| } | ||
| } | ||
| } |
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.
well for big tables this would just crash the app due to performance issues in the worst case scenario. for most cases it would be like a freeze
we need something tricky here. maybe batch search in new thread (service worker)
I suppose not a bad idea to sync with the team how should we handle that so we do not dig into this real hard
also we have react-minisearch lib for searching. maybe it can offer something for us. we need to check the API for ideas
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 simple solution still shows decent results even on huge tables. I would take it to separate ticket for further exploration.
I tried to use Web Worker for that calculations, but the bottleneck here is the getCellText which can't be called somewhere else. Batching, serializing/deserializing, orchestration adds too much overhead and didn't show any significant improvements. Actually, for me it even for worse
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.
We’ll need to come up with a solution anyway. Users with large datasets will definitely run into issues, but we can address performance problems on large datasets in a separate ticket. The main thing for now is that the new functionality should not affect users who don’t use the search and replace feature
| focus: () => panelRef.current?.focus(), | ||
| refresh: () => actions.refresh(), |
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.
better to add it to function declaration in order to improve debug experience
| search: snapshot.query, | ||
| replace: snapshot.replace, | ||
| caseSensitive: snapshot.caseSensitive, | ||
| wholeWord: snapshot.wholeWord, | ||
| regexp: snapshot.regexp, |
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.
maybe it is the same interface? suspiciously 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.
with what interface?
| tableData.editor?.set({ row, column }, value); | ||
| } | ||
|
|
||
| function handleCellChangeBatch(changes: Array<{ rowIdx: number; colIdx: number; value: string }>) { |
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.
strict types pls
| @@ -189,13 +199,20 @@ export const DataGridTable = observer<IDataPresentationProps>(function DataGridT | |||
| return; | |||
| } | |||
|
|
|||
| if (dataGridRef.current?.isReplacing()) { | |||
| return; | |||
| } | |||
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 think we should revert that new behavior
As user I want to see all changes focused. For example we have huge table and our change is out of the screen right now. Currently if I replace 1 value I wont see what was replaced cause it was not scrolled or selected this cell. so we can miss something important
| setMany(updates: Array<{ key: TKey; value: TValue }>): void { | ||
| for (const { key, value } of updates) { | ||
| this.set(key, value); | ||
| } | ||
| } |
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 will cause a lot of actions dispatched and reactions subscribed to this action. so UI maybe can went bananas
maybe we should create a editBatch where we provide multiple edited cells, edit it in batch and execute 1 action
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.
It was not needed actually
| setMany(updates: Array<{ key: TKey; value: TCell }>): void { | ||
| if (updates.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const historyUpdates: Array<{ key: TKey; prevValue: TCell; value: TCell }> = []; | ||
| const actionValues: Array<{ key: TKey; prevValue: TCell; value: TCell }> = []; | ||
|
|
||
| for (const { key, value } of updates) { | ||
| const [update] = this.getOrCreateUpdate(key.row, DatabaseEditChangeType.update); | ||
| const prevValue = update.update[key.column.index] as TCell; | ||
|
|
||
| historyUpdates.push({ key, prevValue, value }); | ||
| actionValues.push({ key, prevValue, value }); | ||
|
|
||
| update.update[key.column.index] = value; | ||
| this.removeEmptyUpdate(update); | ||
| } | ||
|
|
||
| this.historyManager.recordBatchCellEdit({ updates: historyUpdates }); | ||
|
|
||
| this.action.execute({ | ||
| resultId: this.result.id, | ||
| type: DatabaseEditChangeType.update, | ||
| revert: false, | ||
| value: actionValues, | ||
| }); | ||
| } |
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.
like this yes
| hasGlobal(scope: symbol): boolean; | ||
| getGlobal<T>(scope: symbol): T | undefined; | ||
| setGlobal<T>(scope: symbol, value: T): void; | ||
| deleteGlobal(scope: symbol): void; |
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.
maybe better to move it to separate cache action: IDatabaseGlobalCacheAction where we can store global values for the data editor
now it is mixed with cells values so we need to separate abstractions
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'm not sure. We might get confused later by two cache actions :) Let's discuss
…onditional read-only state
| for (let rowIdx = 0; rowIdx < rowCount; rowIdx++) { | ||
| for (let colIdx = 0; colIdx < columnCount; colIdx++) { | ||
| const cellText = getCellText(rowIdx, colIdx); | ||
| if (searchPattern.test(cellText)) { | ||
| matches.push({ rowIdx, colIdx }); | ||
| if (searchPattern.global) { | ||
| searchPattern.lastIndex = 0; | ||
| } | ||
| } | ||
| } | ||
| } |
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.
We’ll need to come up with a solution anyway. Users with large datasets will definitely run into issues, but we can address performance problems on large datasets in a separate ticket. The main thing for now is that the new functionality should not affect users who don’t use the search and replace feature
| } | ||
|
|
||
| /** Replace pattern in cell text. Returns new text and whether pattern still matches. */ | ||
| static replaceInCell(cellText: string, pattern: RegExp, replaceValue: string): { newText: string; stillMatches: boolean } { |
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.
Why do we choose to use static methods and class approach in general here? Seems like it could be 3 functions instead
| }); | ||
|
|
||
| useEffect(() => { | ||
| onCellClassNameChange(getCellClassName); |
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 part is confusing for me. We are reacting to the fn reference change here? Could you describe what its used for please
| onCellKeyDown={handleCellKeyDown} | ||
| onColumnWidthsChange={setColumnWidths} | ||
| /> | ||
| {searchOpen && ( |
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.
We can use <Activity /> here to preserve state and improve perfomance
| return () => el.removeEventListener('keydown', handleKeyDown); | ||
| }, []); | ||
|
|
||
| function handleSearchOpen() { |
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.
Can we move all search related logic to a separate hook? so we dont implement it all in the entry file?
Screen.Recording.2026-01-28.at.15.37.39.mov