-
Notifications
You must be signed in to change notification settings - Fork 411
RI-7614 Introduce shared commands history (for Search and Workbench) #5057
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
- created interface to define the behavior for the services that should fetch commands history data (no matter of the data source) - created example implementation for fetching commands history data from SQLite - created hook to toggle the data source when fetching commands history data, based on a feature flag and target SQLite or INdexDB re #RI-7614
756fab1
to
5f6d57f
Compare
const loadHistory = async () => { | ||
try { | ||
const historyData = await loadHistoryData(instanceId) | ||
const historyData = await getCommandsHistory(instanceId) |
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.
My goal is to preserve the current way of managing command history through the Search page (with this hook) while modifying only the underlying logic responsible for interacting with the data store.
This approach ensures that the existing components remain unchanged, while simultaneously achieving the goal of having an independent data store that can be easily changed via a feature flag.
And of course, by design, this hook (and the components that depend on it) remain unaware of this information, and they will not be affected by any future changes of the data source behind it.
redisinsight/ui/src/services/commands-history/database/CommandsHistoryIndexedDB.ts
Outdated
Show resolved
Hide resolved
redisinsight/ui/src/services/commands-history/hooks/useCommandsHistory.ts
Outdated
Show resolved
Hide resolved
CommandExecutionUI, | ||
} from 'uiSrc/slices/interfaces' | ||
|
||
export interface CommandsHistoryDatabase { |
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 the interface that every service should comply with. It's basic CRUD, simple as that. And it's independent from the data source behind it.
import { mapCommandExecutionToUI } from '../utils/command-execution.mapper' | ||
import { CommandsHistoryDatabase, CommandHistoryResult } from './interface' | ||
|
||
export class CommandsHistorySQLite implements CommandsHistoryDatabase { |
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 an i(example) implementation of the service that should work with the Commands History stored in the SQLite database. It should follow the common interface and keep all of the business login encapsulated inside it.
redisinsight/ui/src/services/commands-history/hooks/useCommandsHistory.ts
Outdated
Show resolved
Hide resolved
redisinsight/ui/src/services/commands-history/utils/command-execution.mapper.ts
Show resolved
Hide resolved
- extend the generic service (and the database related implemnetations) to support adding/running commands - replace the existing implementation for running commands and ssaving the history, on the new Search page re #RI-7614
- it was meant to be a hook only because we need a feature flag to capture which data store shold be used, but after some comments, it's changed to be just a service re #RI-7614
re #RI-7614
re #RI-7614
…r commands history re #RI-7614
localResourcesBaseUrl: process.env.RI_LOCAL_RESOURCES_BASE_URL, | ||
useLocalResources: booleanEnv('RI_USE_LOCAL_RESOURCES', false), | ||
indexedDbName: process.env.RI_INDEXED_DB_NAME || 'RI_LOCAL_STORAGE', | ||
vectorSearchIndexedDbName: |
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.
Adding a separate IndexedDB for the Search page (as advised here).
It will use the same interface like the Workbench, but the Commands History related to these pages will live independently.
|
||
const riConfig = getConfig() | ||
|
||
export const vectorSearchCommandsHistoryStorage = new WorkbenchStorage( |
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.
Adding a separate IndexedDB for the Search page (as advised here).
It will use the same interface like the Workbench, but the Commands History related to these pages will live independently.
type CommandHistoryType = CommandExecution[] | ||
|
||
export async function getLocalWbHistory(dbId: string) { | ||
export async function getLocalWbHistory( |
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.
Extending all the methods to support picking the database instance (as advised here).
The end goal is to keep it simple yet generic, so both the Search and Workbench pages will use the same methods for CRUD related to their Command History stored in IndexedDB.
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.
if all of these methods require a WorkbenchStorage instance as a first parameter, then why not make them methods of WorkbenchStorage class?
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 what the original idea was behind this implementation back then, when it was originally introduced. Maybe it was not considered that one day it would serve two different pages.
In my previous pull request, I have changed the original implementation of the WorkbenchStorage
and it was advised here that it's not the correct approach to extend it. That's why I have followed the provided suggestion in this implementation, simply by extending the helper methods instead of the original class.
import { CommandsHistoryDatabase } from './database/interface' | ||
import { CommandsHistoryIndexedDB } from './database/CommandsHistoryIndexedDB' | ||
|
||
export class CommandsHistoryService { |
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 the main service that should be used when working with Command History data:
- it defines all the necessary CRUD operations for dealing with this type of data
- and it does the magic to target the correct data source, based on a feature flag, hiding these implementation details from its consumers
Code Coverage - Frontend unit tests
Test suite run success5225 tests passing in 681 suites. Report generated by 🧪jest coverage report action from eddc5f3 |
wbHistoryStorage, | ||
WorkbenchStorage, | ||
} from 'uiSrc/services/workbenchStorage' | ||
import { C } from 'msw/lib/glossary-2792c6da' |
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.
nit: I think we don't need this?
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.
Thanks for spotting this, now it's removed 🙌
Description
Introduce a simple abstraction layer for fetching Command History data from any data source.
Currently, we support storing this information in two places:
And we don't have a simple way to CRUD the data in a generic way, without having a highly coupled implementation dependent on the data store. With this pull request, we aim to introduce a way to make this more reusable and easier to extend in the future by creating a common interface that should be independent of the data source used behind it.
How it was tested
Using SQLite
This is the default data store, so when you start the application, go and execute a bunch of queries on both the Search and Workbench pages. The following should work as expected:
Screen.Recording.2025-10-16.at.12.32.25.mov
Using IndexedDB
In order the toggle the data source and make the app use IndexedDB, put the following environment variable.
Then, you can follow the same scenarios as for the SQLite variant.
Screen.Recording.2025-10-16.at.12.38.23.mov
More details
At this point, we do use the Command History on the Search and Workbench pages. And so far, its implementation has been inconsistent:
With this pull request, we aim to introduce a generic way to handle this, but we'll focus on the implementation for the Search page only, which is the problematic one. Later on, we can easily bind the Workbench implementation to the new services, because they're built in a way that can be easily reused across the codebase (or at least, this is the plan)
Notes
Here is a short breakdown of the concepts introduced by this pull request:
PS: There are more comments bellow, scroll down.