Skip to content

feat: search functionality in the tree view with UI updates. Added a …#1

Draft
tribixbite wants to merge 1 commit intoToyKeeper:trunkfrom
tribixbite:feature/search
Draft

feat: search functionality in the tree view with UI updates. Added a …#1
tribixbite wants to merge 1 commit intoToyKeeper:trunkfrom
tribixbite:feature/search

Conversation

@tribixbite
Copy link

…search bar in the side panel, integrated search handling in the TreeView class, and applied search filters to nodes. Updated styles for the search input and results highlighting.

You may want to review: the switch to using index directly instead of local index (~line 390 and 400 in nodeview.js), and $renderWholeTree

…search bar in the side panel, integrated search handling in the TreeView class, and applied search filters to nodes. Updated styles for the search input and results highlighting.
@ToyKeeper
Copy link
Owner

Hi, thanks for the contribution, and for the extra context and screenshot in chat!

i tried to get search going via ai ToyKeeper but gave up after gemini 2.5 pro failed miserably to figure out your code 10 times
claude 4 came out yesterday and fixed all the gemini errors and got it working immediately (first prompt) https://clipthing.com/1747998627.png

I've been meaning to add a filter function eventually, and this should be a big help toward that goal. Also planning to add a search function which navigates to matching items without filtering out non-matching items, to show the matches in context... though that may get tricky when matches are in collapsed branches.

Looking through the patch, it looks like the AI changed a lot of superficial stuff unrelated to the purpose of this patch, like replacing a = a + b with a += b and things like that. It also appears that Claude left comments about Gemini bugs it fixed. There are also a few things renamed for unknown reasons, like "position" became "positionParam" as if it was trying to avoid a reserved keyword or something. Will have to filter those out to get to the relevant changes.

More generally, I haven't figured out yet how to properly handle AI-written contributions. I should probably not block that sort of thing entirely, since it's only going to get more common and more integral to a lot of daily life activities, but the details are still up in the air. Have been considering some sort of contributor agreement asserting that the person contributing code actually wrote the code or at least owns the copyright and thus has the right to submit it, to ensure I don't get in trouble for unknowingly using code which might not be legally in the clear.

It might even be a good idea to include copyright reassignment in the agreement, in case the license might need to change later. Like, GPL didn't handle the case of network services running a proprietary fork, so AGPL was created... and it's possible something like that might happen again with some sort of future "AIGPL" or something, to cover more cases the current license didn't foresee. So I'd like to retain the option of a license change if possible.

Back to the content of the patch, it'll take some time to fully grasp, but I already see some things I'll need to rewrite:

  • There's code added to view.js, and I was planning to delete that file entirely. The code there adds hardcoded key mappings which override and bypass the main key event handler, so I'll definitely need to rewrite that.
  • I'm wondering what the new TreeView.isDisposed thing is for. Was there an issue which this solved?
  • The patch adds a search area above the top bar, and I was planning to implement search within the bottom status bar instead, without adding a dedicated new space to the view for search input. Or perhaps a floating overlay, depending on the details... not sure. I'm trying to avoid further reducing the vertical space available for displaying nodes. One option I'm considering is to extend the "marked nodes count" overlay to handle more things, like an error count and perhaps search status, and maybe move it to the bottom to make it less prone to obscuring tree nodes, and add a bit more margin to the bottom of the tree so it can be scrolled far enough to reach the last node even when the overlay is visible.
  • This search automatically does node.setExpanded() calls to display matching items in collapsed branches. Depending on how the 'searchReveal' reason is handled, that means either a search causes lasting changes to the tree (which shouldn't happen), or the tree view gets desynced from the main tree state. If desynced, it needs to be synced again somehow, or the desyncs need to be tracked, and I don't see any code to do that. In some browsers, the user could close and re-open the tree view to re-sync it, but some browsers (like Vivaldi) don't allow sidepanels to be fully closed and de-instantiated without closing the entire window... so a cleaner solution is needed.
  • I expect the user will do significant amounts of work while a search or filter is active. Like, during a tab de-duplication workflow, they may want to mark several nodes and paste them into a single place, or mark and paste the entire branch where those nodes are contained. So I need to make sure all tree view functions still work while a search or filter is active. But I don't see anything here about handling cursor movement, for example... meaning the user could probably move the cursor to invisible nodes... and then make changes to those invisible nodes.

Testing the submitted code in a browser, I immediately ran into multiple bugs:

  • It failed after 3 keystrokes. I did a Ctrl+F to start a search, then typed foo, and this resulted in a search for "fo" and then o to add a node. This is probably a consequence of bypassing the main key event handler with a separate listener. When I repeated the test 5 times, I got 3 different results, so it looks like they're fighting for focus and getting race conditions.
  • Searching puts the tree in a state where pressing arrow keys moves the cursor to invisible nodes.
  • Matching nodes are found in collapsed branches, but those branches do not actually get expanded in the view, so they remain invisible.
  • The css changes are not compatible with the theme selector; it has dark mode hardcoded.

So this isn't merge-able in its current state. It needs pretty large changes.

@ToyKeeper ToyKeeper marked this pull request as draft May 23, 2025 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants