Feature: autocomplete commands#184
Conversation
arseny-kostenko
left a comment
arseny-kostenko
left a comment
There was a problem hiding this comment.
Thanks for the work on this, but I can’t approve the PR in its current form.
Some feedback is in this comment https://github.com/valkey-io/valkey-admin/pull/184/files#r2700170446
Overall, the implementation lacks modularity and mixes too many responsibilities into single units (notably the autocomplete hook). UI state management, debouncing, matching logic, keyboard navigation, and direct DOM mutation are all tightly coupled, which makes the code hard to reason about, test, and evolve. It also ignores the existing design and tooling.
There’s a lot of imperative, step-by-step logic (“prepare variables now, use them later”) and manual DOM manipulation that fights React’s declarative model. As a result, the code reads as spaghetti rather than a composition of clear, well-defined pieces.
Before this can be merged, I’d like to see:
- Clear separation between text parsing/insertion logic, autocomplete state, and UI rendering.
- Removal of imperative DOM mutations (textarea.value, synthetic input events) in favour of controlled state
- Smaller, focused modules/hooks with single responsibilities — if it's a hook under /hooks directory, it's expected to be a reusable between components hook. Otherwise, if it's just a named function used inside a useEffect/useCallback — it should be close to the component that uses it.
- useEffect and useCallbacks need to be justified
I’m happy to review a refactor along those lines.
|
@arseny-kostenko I really appreciate the thorough review and suggestions. I just pushed a major simplification following your requested guidelines 👍 |
|
Thanks @allenheltondev, this looks way better! I'll do a more thorough review next week. Meanwhile, could you leave some comments around text insertion util — how it works and etc. Thank you! |
| * @param options.maxResults - Maximum number of results | ||
| * @returns Array of match results | ||
| */ | ||
| export function searchCommands( |
There was a problem hiding this comment.
I may be tripping but all these functions are not used in the actual functional code. And some of them are used only in tests. Please remove.
There was a problem hiding this comment.
Unnecessary tests too.
| * @param options.adminMode - Whether to include admin-tier commands (default: false) | ||
| * @returns Filtered list of commands | ||
| */ | ||
| export function getCommands(options: { adminMode?: boolean } = {}): ValkeyCommand[] { |
There was a problem hiding this comment.
getCommands runs on every search and ultimately runs the filter on every search despite static immutable json. I suggest precomputing these values in the module and export them to be used in matchCommands:
const [ADMIN_COMMANDS, NON_ADMIN_COMMANDS] = R.partition(R.propEq("tier", "admin"), ALL_COMMANDS)
There was a problem hiding this comment.
We don't actually have a use case where we need ADMIN_COMMANDS. it's either ALL_COMMANDS or just NON_ADMIN_COMMANDS. So I updated the code appropriately
There was a problem hiding this comment.
getCommands is not used
Signed-off-by: Allen Helton <allenheltondev@gmail.com>
allenheltondev
force-pushed
the
feat/autocomplete-commands
branch
from
00e4813 to
e861714
Compare
|
Thanks for taking the initiative to submit the update. Going forward I would love the first human in the loop for any LLM-assisted changes to be the person submitting the PR. This is also part of our contributing guidelines: I just did an experiment with my LLM to see if it identified that several review points were still unresolved but it took 3 prompts with heavy nudging. LLMs can help generate code very quickly, but someone still has to carefully read and validate the output. If the author doesn't do that validation first, the burden shifts to the reviewer. Thanks again for continuing to work on this, the improvements are appreciated. Also as a side note: the force-push removed the previous commit history from the PR, which made it harder to see what exactly changed between review rounds. In the future it would be preferable to avoid rewriting history during review unless there is a strong reason. not-used.mov |
| * @param options.adminMode - Whether to include admin-tier commands (default: false) | ||
| * @returns Filtered list of commands | ||
| */ | ||
| export function getCommands(options: { adminMode?: boolean } = {}): ValkeyCommand[] { |
There was a problem hiding this comment.
getCommands is not used
- Deleted valkey-command-matching.filtering.test.ts (tests getCommands/searchCommands which don't exist) - Deleted valkey-commands.validation.test.ts (tests getCommands which doesn't exist) - Only matchCommands() exists in the implementation, keeping its valid tests - All remaining tests pass Addresses review feedback from @arseny-kostenko about unused test code. Signed-off-by: Allen Helton <allenheltondev@gmail.com>
Resolved conflicts by accepting incoming changes from main. Signed-off-by: Allen Helton <allenheltondev@gmail.com>
allenheltondev
force-pushed
the
feat/autocomplete-commands
branch
from
e7e064b to
e9806fc
Compare
2 participants
Description
Adding an autocomplete/intellisense for the command page. Works for commands only (no resources), but does tell you the parameters required for the command. This is for issue #151
Change Visualization
Command page with autocomplete open
