-
Notifications
You must be signed in to change notification settings - Fork 68
feat: Concurrent executions with indexedDB #1235
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: feature/distributed-demo
Are you sure you want to change the base?
feat: Concurrent executions with indexedDB #1235
Conversation
I know the test are not working currently. Its just to show the feature, i will work on the test. I am having trouble with getting test of redux to work 😕 |
@Microchesst thanks for the PR. I will get back to you by Monday. |
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.
@Microchesst Thanks for the PR. The feature looks really nice. Please see the comments.
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.
please move all the *.ts files in route/digitaltwins/execute
to model/backend/gitlab/execution
directory. Do remember to
- create an interface for each class so that testing becomes easier. The interfaces related to pipelines can be kept in
model/backend/gitlab/execution/interfaces.ts
- always have
preview/
code depend on stable codebase rather than other way around
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.
Do think about the appropriate location for these slices. Is it src/store
or src/model/store
?
@Microchesst please move your pure typescript files (*.ts) to |
The redux slices can be placed on |
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 adds concurrent execution support for Digital Twins by persisting and managing execution history in IndexedDB and enhancing the UI to display and control multiple runs.
- Migrate digital-twin state and actions to
model/backend/gitlab
slices and introduce IndexedDB schema/service. - Introduce
ExecutionHistoryLoader
for initial load and polling, plusExecutionHistoryList
, updated dialogs/buttons to surface history. - Extend pipeline handlers (
pipelineUtils
,pipelineHandler
,pipelineChecks
) to create, track, and update concurrent executions.
Reviewed Changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
client/src/preview/route/digitaltwins/execute/LogDialog.tsx | Fetch execution history via thunk and render ExecutionHistoryList instead of inline logs |
client/src/preview/route/digitaltwins/editor/Sidebar.tsx | Updated selectDigitalTwinByName import to new model/backend slice |
client/src/preview/route/digitaltwins/create/CreateDTDialog.tsx | Switched setDigitalTwin import to new model/backend slice |
client/src/preview/components/execution/ExecutionHistoryLoader.tsx | New component to load all history on startup and poll running executions |
client/src/preview/components/execution/ExecutionHistoryList.tsx | New list component displaying sorted history with stop/delete controls and log details |
client/src/preview/components/asset/StartStopButton.tsx | Show running count, integrate handleStart , and replace loading indicator |
client/src/preview/components/asset/LogButton.tsx | Add badge count for history entries and disable logic based on execution count |
client/src/preview/components/asset/DetailsButton.tsx | Updated selectDigitalTwinByName import path |
client/src/preview/components/asset/AssetCard.tsx | Initialize logButtonDisabled to false and pass assetName to LogButton |
client/src/preview/components/asset/AssetBoard.tsx | Updated setShouldFetchDigitalTwins import to new model/backend slice |
client/src/model/backend/gitlab/types/executionHistory.ts | New ExecutionStatus , JobLog , ExecutionHistoryEntry , and IndexedDB schema/types |
client/src/model/backend/gitlab/state/executionHistory.slice.ts | New slice with CRUD thunks, selectors, and polling logic for concurrent execution tracking |
client/src/model/backend/gitlab/state/digitalTwin.slice.ts | Aligned JobLog type import and added selectDigitalTwins selector |
client/src/model/backend/gitlab/execution/pipelineUtils.ts | Refactored pipeline state helpers to support execution history updates and concurrent flows |
client/src/model/backend/gitlab/execution/pipelineHandler.ts | Updated handleStart /handleStop to dispatch history fetch and pass executionId |
client/src/model/backend/gitlab/execution/pipelineChecks.ts | Enhanced status checks to update IndexedDB, handle timeouts, and use new interfaces |
client/src/model/backend/gitlab/execution/interfaces.ts | Defined PipelineStatusParams and PipelineHandlerDispatch for typed thunk usage |
client/src/database/digitalTwins.ts | IndexedDB service implementation conforming to new schema for persisting execution history |
client/src/AppProvider.tsx | Registered ExecutionHistoryLoader at the app root |
client/package.json | Added uuid , @types/uuid , fake-indexeddb and updated dependencies for IndexedDB and IDs |
Comments suppressed due to low confidence (3)
client/src/preview/components/execution/ExecutionHistoryLoader.tsx:15
ExecutionHistoryLoader
handles initial data fetch and polling but has no associated unit or integration tests. Consider adding tests to cover its effect logic and cleanup of the interval.
const ExecutionHistoryLoader: React.FC = () => {
client/src/preview/route/digitaltwins/execute/LogDialog.tsx:27
- [nitpick] The component is named
LogDialog
but now presents execution history. Consider renaming it toExecutionHistoryDialog
for clarity and consistency with its content.
function LogDialog({ showLog, setShowLog, name }: LogDialogProps) {
client/src/model/backend/gitlab/execution/pipelineChecks.ts:76
- Calling
digitalTwin.getExecutionHistoryById
assumes theDigitalTwin
class exposes that method, but IndexedDB access should useindexedDBService
. Replace this withindexedDBService.getExecutionHistoryById
to avoid runtime errors.
const pipelineId = executionId
const setButtonText = () => {}; // Dummy function since we don't need to change button text | ||
handleStart( | ||
'Start', |
Copilot
AI
May 24, 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 call to handleStart
always passes 'Start'
and you’ve removed any buttonText
state, so the UI can never switch to 'Stop'. This breaks the stop functionality—consider managing a local buttonText
state or passing the current executionId to toggle correctly.
const setButtonText = () => {}; // Dummy function since we don't need to change button text | |
handleStart( | |
'Start', | |
const newButtonText = buttonText === 'Start' ? 'Stop' : 'Start'; | |
setButtonText(newButtonText); | |
handleStart( | |
newButtonText, |
Copilot uses AI. Check for mistakes.
} | ||
}, [dispatch, name, showLog]); | ||
|
||
const handleViewLogs = () => {}; |
Copilot
AI
May 24, 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.
handleViewLogs
is defined with no parameters but ExecutionHistoryList
expects onViewLogs
to accept an executionId: string
. Either implement log fetching there or remove the unused callback.
const handleViewLogs = () => {}; | |
const handleViewLogs = (executionId: string) => { | |
console.log(`View logs for execution ID: ${executionId}`); | |
}; |
Copilot uses AI. Check for mistakes.
(jobLog: JobLog, index: number) => ( | ||
<div key={index} style={{ marginBottom: '16px' }}> |
Copilot
AI
May 24, 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.
[nitpick] Using the array index as a React key can lead to rendering issues if logs change order or new logs are inserted. Use a stable identifier (e.g., jobLog.jobName
or a combined ID) instead.
(jobLog: JobLog, index: number) => ( | |
<div key={index} style={{ marginBottom: '16px' }}> | |
(jobLog: JobLog) => ( | |
<div key={jobLog.jobName} style={{ marginBottom: '16px' }}> |
Copilot uses AI. Check for mistakes.
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.
@Microchesst Thanks for the updates. The feature functionality is much improved and it works as discussed. However, I have a few comments on the existing implementation. Please check the comments. Thanks.
client/src/database/digitalTwins.ts
Outdated
* Interface for IndexedDB operations | ||
*/ | ||
class IndexedDBService { | ||
export interface IIndexedDBService { |
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 the name is changed to IExecutionHistory
, the ExecutionHistory
suffix can be removed from the method names.
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 rest of the code need not know about the use of IndexDB. The interface can be independent of IndexDB and the ExecutionHistory
class can include IndexDB inside without letting other parts of the code knowing about it.
<ThemeProvider theme={mdTheme}> | ||
<AuthProvider> | ||
<CssBaseline /> | ||
<ExecutionHistoryLoader /> |
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 is this react component here?
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 of the methods are returning Promise<void>
. There are other possible outcomes with the IndexDB operations. Perhaps they should be used to provide meaningful promises?
pipelineId: number; // GitLab pipeline ID | ||
timestamp: number; // Timestamp when the execution was started | ||
status: ExecutionStatus; // Current status of the execution | ||
jobLogs: JobLog[]; // Logs from the execution |
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.
Is the array collecting logs separately from each of the jobs in a pipeline?
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.
Previously one button was used for the start and stop actions but now they are separate. The stop is inside History button. Perhaps a HistoryButton.tsx
component be placed in src/components/asset
. The component responsibility could be:
StartButton.tsx --> manages only start
HistoryButton.tsx --> manages stop and logs
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.
please move this to src/components/execution
|
||
const intervalId = setInterval(() => { | ||
dispatch(checkRunningExecutions()); | ||
}, 10000); // Check every 10 seconds |
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.
please give a name to constant and place it in src/model/backend/gitlab/constants.ts
import { RootState } from 'store/store'; | ||
import { addOrUpdateLibraryFile } from 'preview/store/libraryConfigFiles.slice'; | ||
import { getFilteredFileNames } from 'preview/util/fileUtils'; | ||
import { FileState } from '../../../store/file.slice'; |
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.
paths from src
directory
client/src/store/store.ts
Outdated
'assets/setAsset', | ||
'assets/deleteAsset', | ||
// Execution history actions | ||
'executionHistory/addExecutionHistoryEntry', |
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 this part be simplified but refactoring the executionHistory.slice
and digitaltwin.slice
?
|
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.
@Microchesst you are right about growing size of this PR. There are too many dependencies from src/route/digitaltwins/execution to src/preview. It makes sense to move the dependent files to corresponding locations in src/preview and then proceed with the PR. Hope that decision reduces the time to complete the PR.
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.
please rename file to executionStatusHandlers.ts and variables to
- setButtonText to setLogStatusText
- LogButtonDisabled to LogDisabled
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.
These files are importing from preview/. It might take too much of refactoring to remove dependence from preview/ code. If it is so, please place these files in the right location in preview/ itself.
(entries) => entries.find((entry) => entry.id === id), | ||
); | ||
|
||
export const selectSelectedExecutionId = (state: RootState) => |
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.
odd name
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 file depends on IndexDB. Perhaps this should be moved to src/services
?
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.
perhaps moving this preview/util will reduce the time required to complete this PR
Pull Request Template
Title
Implement Concurrent Digital Twin Execution with IndexedDB Integration
Type of Change
Description
This PR implements concurrent execution for Digital Twins with IndexedDB integration. Users can now start multiple executions of the same Digital Twin simultaneously, with execution history persisted in the browser's IndexedDB. The implementation includes a new execution history data model, IndexedDB service, and UI components to display and manage execution history.
##Working on Testing
Impact
Checklist