automations #1343
automations #1343
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a comprehensive refactoring of the application's integration and automation capabilities. The changes replace the existing OSC and HTTP integration services with a new, more flexible automation system. The new architecture allows for creating and managing automation blueprints and rules that can trigger actions based on timer lifecycle events. The modifications span both client and server-side code, introducing new components for managing automation settings, creating blueprints, and handling output dispatching. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
cpvalente
force-pushed
the
integrations
branch
3 times, most recently
from
e6ee477 to
cd34149
Compare
cpvalente
force-pushed
the
integrations
branch
3 times, most recently
from
a20ed53 to
1264147
Compare
cpvalente
force-pushed
the
integrations
branch
14 times, most recently
from
6409c38 to
717a1ce
Compare
cpvalente
force-pushed
the
integrations
branch
6 times, most recently
from
399fe0e to
e3246f5
Compare
|
|
||
| export const automationPlaceholderSettings: AutomationSettings = { | ||
| enabledAutomations: false, | ||
| enabledOscIn: false, |
There was a problem hiding this comment.
@alex-Arc i am wondering if the OSC settings should be in the app settings instead of here
What do you think?
There was a problem hiding this comment.
yes
at least it should be separate from the automations
| const fieldValue = state[field]; | ||
|
|
||
| // TODO: if value is empty string, the user could be meaning to check if the value does not exist | ||
| switch (operator) { |
There was a problem hiding this comment.
@alex-Arc i have been wondering if we could come up with a few examples and fine tune the operators?
| * Expose possibility to send a message using OSC protocol | ||
| */ | ||
| export function emitOSC(output: OSCOutput, state: RuntimeState) { | ||
| const message = preparePayload(output, state); |
cpvalente
force-pushed
the
integrations
branch
2 times, most recently
from
8f6dbec to
55bd741
Compare
| const handleTestOSCOutput = async (index: number) => { | ||
| try { | ||
| const values = getValues(`outputs.${index}`) as OSCOutput; | ||
| if (!values.targetIP || !values.targetPort || !values.address || !values.args) { |
There was a problem hiding this comment.
This if statement currently prevents testing OSC output if no args are specified.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
🔭 Outside diff range comments (1)
apps/server/src/stores/EventStore.ts (1)
Line range hint
1-44: Consider adding error handling for WebSocket failures.The store broadcasts state changes via WebSocket but doesn't handle potential connection failures or errors.
Add error handling:
set<T extends keyof RuntimeStore>(key: T, value: RuntimeStore[T]) { store[key] = value; - socket.sendAsJson({ - type: `ontime-${key}`, - payload: value, - }); + try { + socket.sendAsJson({ + type: `ontime-${key}`, + payload: value, + }); + } catch (error) { + console.error('Failed to broadcast store update:', error); + // Consider implementing retry logic or error reporting + } }
🧹 Nitpick comments (54)
apps/server/src/classes/data-provider/DataProvider.utils.ts (1)
Line range hint
3-5: Update JSDoc to reflect automation capabilities.The current documentation is too generic and doesn't mention the automation aspect of the merge operation.
Consider updating the JSDoc to:
/** - * Merges a partial ontime project into a given ontime project + * Merges a partial ontime project into a given ontime project, handling automation settings + * and other project configurations while preserving existing data where not overridden. */apps/server/src/api-data/automation/clients/http.client.ts (2)
8-14: Enhance JSDoc documentation for better clarity.The JSDoc could be more descriptive by including:
/** * Expose possibility to send a message using HTTP protocol + * @param output - The HTTP output configuration containing the URL template + * @param state - The current runtime state used for template parsing + * @example + * emitHTTP({ url: 'http://example.com/${timer}' }, { timer: '10:00' }) */
16-20: Add URL validation before returning.The function should validate the parsed URL to ensure it's well-formed before returning.
function preparePayload(output: HTTPOutput, state: RuntimeState): string { const parsedUrl = parseTemplateNested(output.url, state); + try { + new URL(parsedUrl); // Validate URL format + } catch (error) { + logger.warning(LogOrigin.Rx, `Invalid URL format: ${parsedUrl}`); + throw new Error(`Invalid URL format: ${parsedUrl}`); + } return parsedUrl; }apps/server/src/classes/data-provider/__tests__/DataProvider.utils.test.ts (1)
126-131: Consider extracting the default automation configuration into a shared test fixture.The automation configuration is duplicated between test cases. Consider extracting it into a shared fixture or helper function to maintain DRY principles and make future updates easier.
+ const DEFAULT_AUTOMATION_CONFIG = { + enabledAutomations: false, + enabledOscIn: false, + oscPortIn: 8000, + automations: [], + blueprints: {}, + }; const existingData = { rundown: [], // ... other properties - automation: { - enabledAutomations: false, - enabledOscIn: false, - oscPortIn: 8000, - automations: [], - blueprints: {}, - }, + automation: DEFAULT_AUTOMATION_CONFIG, } as DatabaseModel;apps/server/src/utils/parserFunctions.ts (1)
Line range hint
251-252: Track the technical debt for removing lowercase key compatibility.The TODO comment indicates a temporary backwards compatibility measure that can be removed in October 2024. Consider creating a tracking issue to ensure this technical debt is addressed at the appropriate time.
Would you like me to create a GitHub issue to track the removal of lowercase key compatibility in October 2024?
apps/client/src/features/app-settings/panel-utils/PanelUtils.module.scss (1)
194-208: LGTM! Well-structured modifiers with semantic naming.The renaming from
.inlineSiblingsto.inlineElementsand the addition of modifiers improves clarity and flexibility. The BEM-like structure makes the code more maintainable.Consider extracting the gap values into SCSS variables for better maintainability:
+$gap-small: 0.5rem; +$gap-medium: 1rem; .inlineElements { display: flex; align-items: center; &.inner { - gap: 0.5rem; + gap: $gap-small; } &.component { - gap: 1rem; + gap: $gap-medium; }apps/server/src/api-data/automation/automation.router.ts (5)
13-13: Consider removing the.jsextension from import paths.TypeScript typically doesn't require
.jsextensions in import paths as they are handled by the TypeScript compiler.-} from './automation.controller.js'; +} from './automation.controller';
26-27: Consider adding error handling middleware.While the routes are well-structured, they might benefit from error handling middleware to ensure consistent error responses.
Example implementation:
const errorHandler = (err: Error, req: Request, res: Response, next: NextFunction) => { console.error(err.stack); res.status(500).json({ error: 'Internal Server Error' }); }; router.use(errorHandler);Also, consider adding validation for the GET endpoint to ensure query parameters are valid if any are expected.
33-35: Consider adding GET endpoints and reviewing route naming consistency.
Missing GET endpoints for:
- List all blueprints (
GET /blueprint)- Get a specific blueprint (
GET /blueprint/:id)Consider using plural form for resource names to follow REST conventions:
-router.post('/blueprint', validateBlueprint, postBlueprint); -router.put('/blueprint/:id', validateBlueprintPatch, editBlueprint); -router.delete('/blueprint/:id', paramContainsId, deleteBlueprint); +router.post('/blueprints', validateBlueprint, postBlueprint); +router.put('/blueprints/:id', validateBlueprintPatch, editBlueprint); +router.delete('/blueprints/:id', paramContainsId, deleteBlueprint);
37-37: Consider using a more specific route path for the test endpoint.The current
/testpath is quite generic. Consider making it more specific to better reflect its purpose:-router.post('/test', validateTestPayload, testOutput); +router.post('/test-output', validateTestPayload, testOutput);
1-37: Well-structured router implementation with clear separation of concerns.The router implementation successfully achieves the PR objectives by:
- Separating validation logic from route handlers
- Providing a clean API structure for automation and blueprints
- Supporting the new filtering logic through appropriate endpoints
The architecture is solid and maintainable.
Consider documenting the API endpoints using OpenAPI/Swagger for better API documentation and client generation.
apps/server/src/app.ts (3)
Line range hint
92-95: Update theOntimeStartOrderenum to reflect current initialization steps.Since
startIntegrationsand its related initialization stepInitIOare no longer needed, consider removingInitIOfrom theOntimeStartOrderenum to keep it consistent with the current initialization process.Apply this diff to update the enum:
enum OntimeStartOrder { Error, InitAssets, InitServer, - InitIO, }
Line range hint
188-190: Consider removing the now-unusedstartIntegrationsfunction.The
startIntegrationsfunction is currently empty, indicating that it may no longer be necessary after the refactoring. Removing unused functions helps keep the codebase clean and maintainable.Apply this diff to remove the unused function:
-export const startIntegrations = async () => { - checkStart(OntimeStartOrder.InitIO); -};
Line range hint
100-109: Revise thecheckStartfunction for consistency with updated start order.With the removal of
InitIO, ensure that thecheckStartfunction's logic is updated accordingly. Specifically, check the conditions and transitions related toOntimeStartOrderto prevent any potential errors due to the change in initialization steps.apps/server/src/api-data/automation/automation.service.ts (2)
76-99: Ensure type safety when comparingfieldValueandvalue.Currently, the code uses loose equality
==and!=to comparefieldValueandvalue, allowing for type coercion (e.g.,'10' == 10). This can lead to unexpected behavior and bugs due to implicit type conversions.Consider using strict equality
===and!==for more predictable comparisons, or explicitly convert types before comparison to ensure the values are of the same type.Apply this diff to use strict equality:
switch (operator) { case 'equals': - return fieldValue == value; + return fieldValue === value; case 'not_equals': - return fieldValue != value; + return fieldValue !== value;
93-96: Extend handling of non-stringfieldValuein 'contains' and 'not_contains' operators.The 'contains' and 'not_contains' operators currently check if
fieldValueis a string before usingincludes(value). IffieldValueis not a string, the function returnsfalse.Consider supporting additional types, such as arrays, to make the condition evaluation more robust. For example, if
fieldValuecould be an array, you might want to check if it includes thevalue.Apply this diff to handle arrays:
case 'contains': - return typeof fieldValue === 'string' && fieldValue.includes(value); + if (typeof fieldValue === 'string') { + return fieldValue.includes(value); + } + if (Array.isArray(fieldValue)) { + return fieldValue.includes(value); + } + return false; case 'not_contains': - return typeof fieldValue === 'string' && !fieldValue.includes(value); + if (typeof fieldValue === 'string') { + return !fieldValue.includes(value); + } + if (Array.isArray(fieldValue)) { + return !fieldValue.includes(value); + } + return false;apps/client/src/features/app-settings/panel/automations-panel/BlueprintForm.tsx (4)
285-410: Refactor to reduce code duplication in output forms rendering.The code blocks for rendering OSC and HTTP outputs are similar, with variations tailored to their specific fields. Consider abstracting common functionality into reusable components or custom hooks to reduce duplication, improve maintainability, and enhance readability.
97-113: Provide user feedback on errors during OSC output testing.Currently, errors during OSC output testing are silently ignored, which may lead to user confusion if the test fails. Consider handling errors by displaying a notification or an error message to inform the user of any issues encountered.
115-129: Provide user feedback on errors during HTTP output testing.Errors during HTTP output testing are not communicated to the user. Improving error handling by providing user feedback will enhance the user experience and help in troubleshooting configuration issues.
245-265: Remove redundantisDisabledandisLoadingproperties set tofalse.Properties like
isDisabled={false}andisLoading={false}are unnecessary because their default values arefalse. Removing them can simplify the code and improve readability.Apply this diff to remove redundant properties:
- isDisabled={false} - isLoading={false}Also applies to: 350-365, 396-410
apps/server/src/models/dataModel.ts (1)
34-40: Add type definitions and validation for automation settings.While the structure looks good, consider these improvements:
- Add TypeScript interfaces for
automationsandblueprints.- Add validation for
oscPortIn(typical range: 1024-65535).- Document the purpose of these default values.
Example type definitions:
interface AutomationBlueprint { // Add blueprint properties } interface Automation { // Add automation properties } interface AutomationSettings { enabledAutomations: boolean; enabledOscIn: boolean; oscPortIn: number; automations: Automation[]; blueprints: Record<string, AutomationBlueprint>; }apps/server/src/stores/__mocks__/runtimeState.mocks.ts (1)
44-45: Consider safer type handling for merged state.The type assertion
as RuntimeStatemight hide potential type mismatches. Consider:
- Using a type guard to validate the merged state.
- Or using a typed version of deepmerge.
Example with type guard:
function isRuntimeState(obj: unknown): obj is RuntimeState { // Add runtime state validation return true; // implement validation logic } export function makeRuntimeStateData(patch?: Partial<RuntimeState>): RuntimeState { const merged = deepmerge(baseState, patch); if (!isRuntimeState(merged)) { throw new Error('Invalid runtime state after merge'); } return merged; }apps/client/src/features/app-settings/panel/automations-panel/__tests__/automationUtils.test.ts (1)
7-11: Consider adding edge cases to the test suite.The current test suite could benefit from additional edge cases:
- Empty array input
- Single entry array
- Case sensitivity in titles
- Multiple sets of duplicates
apps/client/src/theme/ontimeRadio.ts (1)
3-9: Consider extracting color constants.The color codes are currently hard-coded with comments indicating their semantic names. Consider extracting these into theme constants for better maintainability and consistency.
+// In theme/colors.ts +export const colors = { + gray1200: '#262626', + uiWhite: '#f6f6f6', + // ... other colors +}; +// In ontimeRadio.ts control: { - borderColor: '#262626', // $gray-1200 + borderColor: colors.gray1200, // ... apply similar changes to other color values }apps/server/src/utils/assert.ts (1)
31-40: Consider enhancing thehasKeysfunction.The implementation is solid but could be improved with:
- Type safety for nested keys
- Optional keys support
- Value type validation
Example enhancement:
export function hasKeys< T extends object, K extends keyof any, Required extends K[] = K[], Optional extends K[] = K[] >( value: T, required: Required, optional: Optional = [] as Optional, ): asserts value is T & Record<Required[number], unknown> & Partial<Record<Optional[number], unknown>> { for (const key of required) { if (!(key in value)) { throw new Error(`Required key not found: ${String(key)}`); } } }apps/client/src/common/hooks-query/useAutomationSettings.ts (1)
15-16: Consider adjusting the retry delay strategy.The exponential backoff (attempt * 2500ms) might lead to very long delays for later retry attempts. For example, the 5th retry would wait 12.5 seconds. Consider using a more moderate strategy or capping the maximum delay.
- retry: 5, - retryDelay: (attempt: number) => attempt * 2500, + retry: 3, + retryDelay: (attempt: number) => Math.min(attempt * 1000, 3000),apps/client/src/features/app-settings/panel/automations-panel/automationUtils.ts (1)
34-51: Optimize duplicate checking logic.The current implementation can be simplified using a Set for better performance and readability.
export function checkDuplicates(automations: Automation[]) { - const automationMap: Record<string, string[]> = {}; + const seen = new Set<string>(); const duplicates = []; for (let i = 0; i < automations.length; i++) { const automation = automations[i]; - if (!Object.hasOwn(automationMap, automation.trigger)) { - automationMap[automation.trigger] = []; - } + const key = `${automation.trigger}-${automation.blueprintId}`; - if (automationMap[automation.trigger].includes(automation.blueprintId)) { + if (seen.has(key)) { duplicates.push(i); } else { - automationMap[automation.trigger].push(automation.blueprintId); + seen.add(key); } } return duplicates.length > 0 ? duplicates : undefined; }apps/client/src/common/api/automation.ts (2)
36-39: Add response type validation for mutation endpoints.The mutation endpoints should validate the response data. Consider adding a validation layer:
function validateAutomation(data: unknown): Automation { if (!isAutomation(data)) { throw new Error('Invalid automation data received from server'); } return data; } export async function addAutomation(automation: AutomationDTO): Promise<Automation> { const res = await axios.post<unknown>(`${automationsPath}/automation`, automation); return validateAutomation(res.data); }Also applies to: 44-47, 59-62, 67-70
83-85: Add timeout for test output endpoint.The test endpoint should have a reasonable timeout to prevent hanging:
export function testOutput(output: AutomationOutput): Promise<void> { return axios.post(`${automationsPath}/test`, output, { timeout: 5000, // 5 seconds }); }apps/client/src/features/app-settings/panel/automations-panel/AutomationsListItem.tsx (2)
13-22: Add JSDoc documentation for the interface.Consider adding JSDoc documentation to describe the purpose and usage of each prop in the
AutomationsListItemPropsinterface. This will improve code maintainability and help other developers understand how to use this component.+/** + * Props for the AutomationsListItem component + * @property {NormalisedAutomationBlueprint} blueprints - Collection of available automation blueprints + * @property {string} id - Unique identifier for the automation + * @property {string} title - Display title for the automation + * @property {TimerLifeCycle} trigger - Event that triggers the automation + * @property {string} blueprintId - ID of the associated blueprint + * @property {boolean} [duplicate] - Flag indicating if this is a duplicate automation + * @property {() => void} handleDelete - Callback for deletion + * @property {() => void} postSubmit - Callback after successful form submission + */ interface AutomationsListItemProps { blueprints: NormalisedAutomationBlueprint; id: string;
31-31: Replace arbitrary colSpan value.The
colSpan={99}is an arbitrary value that might not work correctly with all table layouts. Consider using a more semantic value or calculating it based on the actual number of columns.- <td colSpan={99}> + <td colSpan={4}> {/* Based on the actual number of columns in the table */}apps/client/src/features/app-settings/panel/automations-panel/AutomationsList.tsx (4)
15-18: Consider enhancing type safety for the props interface.The interface could benefit from making the props more specific:
interface AutomationsListProps { - automations: Automation[]; + automations: readonly Automation[]; - blueprints: NormalisedAutomationBlueprint; + blueprints: Readonly<NormalisedAutomationBlueprint>; }
26-34: Clear error state before new deletion attempts.The error state should be cleared when starting a new deletion to prevent stale errors from being displayed.
const handleDelete = async (id: string) => { + setDeleteError(null); try { await deleteAutomation(id); } catch (error) { setDeleteError(maybeAxiosError(error)); } finally { refetch(); } };
65-69: Fix grammatical error in duplicate warning message.The error message for duplicates has a grammatical error.
{duplicates && ( <Panel.Error> - You have created multiple links between the same trigger and blueprint which can performance issues. + You have created multiple links between the same trigger and blueprint which can cause performance issues. </Panel.Error> )}
102-108: Simplify delete error display logic.The delete error display can be simplified by moving it outside the mapping function since it's not specific to any automation.
- {deleteError && ( - <tr> - <td colSpan={5}> - <Panel.Error>{deleteError}</Panel.Error> - </td> - </tr> - )}And add it after the mapping:
+ {deleteError && ( + <tr> + <td colSpan={5}> + <Panel.Error>{deleteError}</Panel.Error> + </td> + </tr> + )}apps/client/src/features/app-settings/panel-utils/PanelUtils.tsx (2)
Line range hint
65-74: Consider enhancing type safety for TableEmpty props.The props could benefit from more specific types and documentation.
-export function TableEmpty({ label, handleClick }: { label?: string; handleClick?: () => void }) { +interface TableEmptyProps { + /** Custom label to display. Defaults to 'No data yet' */ + label?: string; + /** Optional click handler. Button is disabled when not provided */ + handleClick?: () => void; +} + +export function TableEmpty({ label, handleClick }: TableEmptyProps) {
127-144: Enhance type constraints and add JSDoc documentation.The InlineElements component could benefit from better type constraints and documentation.
+/** + * Renders inline elements with customizable HTML tag and alignment. + * @template C - The HTML tag to use for rendering + */ type AllowedInlineTags = 'div' | 'td'; type InlineProps<C extends AllowedInlineTags> = { + /** Optional HTML tag to use. Defaults to 'div' */ as?: C; + /** Defines the relation type for styling. Defaults to 'component' */ relation?: 'inner' | 'component' | 'section'; + /** Alignment of the inline elements. Defaults to 'start' */ align?: 'start' | 'end'; className?: string; -}; +} & Omit<JSX.IntrinsicElements[C], keyof InlineProps<C>>;apps/client/src/features/app-settings/panel/automations-panel/BlueprintsList.tsx (2)
26-64: Consider improving error handling and state updates.Two suggestions for enhancing the implementation:
- Consider implementing optimistic updates for delete operations to improve perceived performance
- Make error states blueprint-specific instead of component-wide to avoid confusion when multiple operations fail
-const [deleteError, setDeleteError] = useState<string | null>(null); +const [deleteErrors, setDeleteErrors] = useState<Record<string, string>>({}); const handleDelete = async (id: string) => { try { - setDeleteError(null); + setDeleteErrors(prev => ({ ...prev, [id]: null })); await deleteBlueprint(id); + // Optimistic update + const newBlueprints = { ...blueprints }; + delete newBlueprints[id]; + // Update local state immediately + setBlueprintFormData(null); } catch (error) { - setDeleteError(maybeAxiosError(error)); + setDeleteErrors(prev => ({ ...prev, [id]: maybeAxiosError(error) })); } };
66-126: Enhance accessibility and theming.Consider the following improvements:
- Add ARIA labels and roles to the table for better screen reader support
- Move hard-coded colors to the theme system
-<Panel.Table> +<Panel.Table role="grid" aria-label="Automation blueprints"> <thead> - <tr> + <tr role="row"> - <th style={{ width: '45%' }}>Title</th> + <th role="columnheader" style={{ width: '45%' }}>Title</th> - color='#e2e2e2' // $gray-200 + color='gray.200' - color='#FA5656' // $red-500 + color='red.500'apps/client/src/features/app-settings/panel/automations-panel/AutomationSettingsForm.tsx (2)
38-45: Enhance error handling specificity.The current error handling sets a root-level error message. Consider handling specific error cases differently.
const onSubmit = async (formData: AutomationSettingsProps) => { try { await editAutomationSettings(formData); } catch (error) { const message = maybeAxiosError(error); - setError('root', { message }); + if (message.includes('port')) { + setError('oscPortIn', { message }); + } else if (message.includes('automation')) { + setError('enabledAutomations', { message }); + } else { + setError('root', { message }); + } } };
146-154: LGTM! Comprehensive port validation.The validation rules for the OSC port are well-implemented with proper range checks.
Consider adding a more user-friendly message for the numeric pattern validation:
pattern: { value: isOnlyNumbers, - message: 'Value should be numeric', + message: 'Port number must contain only digits', },apps/server/src/api-data/automation/automation.validation.ts (2)
27-42: Extract common validation patterns.Consider creating reusable validation chains to reduce code duplication.
+const commonValidations = { + title: body('title').exists().isString().trim(), + trigger: body('trigger').exists().isIn(timerLifecycleValues), + blueprintId: body('blueprintId').exists().isString().trim(), +}; export const validateAutomationSettings = [ body('enabledAutomations').exists().isBoolean(), body('enabledOscIn').exists().isBoolean(), body('oscPortIn').exists().isPort(), body('automations').optional().isArray(), - body('automations.*.title').optional().isString().trim(), - body('automations.*.trigger').optional().isIn(timerLifecycleValues), - body('automations.*.blueprintId').optional().isString().trim(), + body('automations.*').optional().custom((value) => { + commonValidations.title.run(value); + commonValidations.trigger.run(value); + commonValidations.blueprintId.run(value); + return true; + }), body('blueprints').optional().custom(parseBluePrint),
132-167: Enhance error messages for output validation.The current error messages could be more descriptive to help users identify and fix issues.
- throw new Error('Invalid automation'); + throw new Error(`Invalid output type: ${type}. Supported types are 'osc' and 'http'`); function validateOSCOutput(payload: object): payload is OSCOutput { - assert.hasKeys(payload, ['targetIP', 'targetPort', 'address', 'args']); + const requiredKeys = ['targetIP', 'targetPort', 'address', 'args']; + assert.hasKeys(payload, requiredKeys, `OSC output missing required fields: ${requiredKeys.join(', ')}`); if (typeof args !== 'string' && typeof args !== 'number') { - throw new Error('Invalid automation'); + throw new Error('OSC args must be either a string or a number'); }apps/server/src/api-data/automation/__tests__/automation.dao.test.ts (1)
42-232: Consider adding more edge cases to the test suite.The test coverage is good, but consider adding these scenarios:
- Test validation of malformed blueprint data
- Test concurrent modifications
- Test maximum length constraints
Example additional test:
it('should reject malformed blueprint data', () => { const invalidData = { title: '', // Empty title filterRule: 'invalid_rule', filters: null, // Invalid filters outputs: [{ type: 'unknown' }], // Invalid output type }; expect(() => addBlueprint(invalidData)).toThrow(); });apps/server/src/api-data/automation/__tests__/automation.utils.test.ts (1)
169-241: Add edge case tests for stringToOSCArgs.While the current test coverage is good, consider adding tests for the following edge cases:
- Invalid number formats
- Mixed case boolean values (e.g., "True", "FALSE")
- Malformed quoted strings (unclosed quotes)
apps/client/src/features/app-settings/panel/automations-panel/BlueprintForm.module.scss (2)
27-50: Consider extracting common styles to reduce duplication.Multiple sections share identical styles for labels. Consider extracting these common styles into a shared class to improve maintainability.
+.formLabel { + font-size: calc(1rem - 3px); + color: $label-gray; +} .titleSection, .ruleSection, .filterSection, .oscSection, .httpSection, .companionSection { display: grid; grid-gap: 0.5rem; button { align-self: end; } - - label { - font-size: calc(1rem - 3px); - color: $label-gray; - } + + label { + @extend .formLabel; + } }
57-67: Consider responsive design for grid layouts.The fixed grid column sizes might not work well on smaller screens. Consider adding media queries to adjust the grid layout for different viewport sizes.
.filterSection { grid-template-columns: 2fr 1fr 2fr auto; + + @media (max-width: 768px) { + grid-template-columns: 1fr; + gap: 1rem; + } } .oscSection { grid-template-columns: 9rem 5rem 3fr 4fr auto; + + @media (max-width: 768px) { + grid-template-columns: 1fr; + gap: 1rem; + } }apps/client/src/common/utils/__tests__/regex.test.ts (1)
29-30: Enhance test coverage for URL validation.While adding HTTPS support is good, consider adding more edge cases to improve test coverage:
- URLs with different paths and query parameters
- URLs with ports
- URLs with special characters
- const right = ['https://test', 'http://test']; - const wrong = ['testing', '123.0.1']; + const right = [ + 'https://test', + 'http://test', + 'https://test:8080', + 'http://test/path?query=1', + 'https://sub.domain.com' + ]; + const wrong = [ + 'testing', + '123.0.1', + 'ftp://test', + 'httptest', + 'http:/test' + ];apps/client/src/features/app-settings/panel/automations-panel/AutomationForm.tsx (3)
31-36: Consider enhancing type safety with explicit form values type.Instead of using
AutomationDTOdirectly, consider creating a specific form values type that matches your form fields exactly. This will help catch potential mismatches between form fields and DTO properties early.- } = useForm<AutomationDTO>({ + type AutomationFormValues = Pick<AutomationDTO, 'title' | 'trigger' | 'blueprintId'>; + } = useForm<AutomationFormValues>({
44-63: Enhance error handling and type safety in form submission.The error handling could be improved by:
- Adding specific error types for different failure scenarios
- Providing more detailed error messages to users
- Handling network timeouts
const onSubmit = async (values: AutomationDTO) => { + const timeout = 5000; // 5 seconds if (initialId) { try { - await editAutomation(initialId, { id: initialId, ...values }); + await Promise.race([ + editAutomation(initialId, { id: initialId, ...values }), + new Promise((_, reject) => + setTimeout(() => reject(new Error('Request timed out')), timeout) + ), + ]); postSubmit(); } catch (error) { + const errorMessage = error instanceof Error && error.message === 'Request timed out' + ? 'Failed to save changes: Request timed out' + : `Failed to save changes to automation: ${maybeAxiosError(error)}`; - setError('root', { message: `Failed to save changes to automation ${maybeAxiosError(error)}` }); + setError('root', { message: errorMessage }); }
77-119: Enhance form accessibility.Consider adding the following accessibility improvements:
- ARIA labels for form controls
- Error message association with form fields
- Better focus management
<label> Title <Input + aria-label="Automation title" + aria-describedby="title-error" {...register('title', { required: { value: true, message: 'Required field' } })} size='sm' variant='ontime-filled' autoComplete='off' defaultValue={initialTitle} /> - <Panel.Error>{errors.title?.message}</Panel.Error> + <Panel.Error id="title-error">{errors.title?.message}</Panel.Error> </label>apps/server/src/api-data/automation/automation.utils.ts (3)
8-14: Strengthen type validation functions.The validation functions could be more robust by:
- Using type guards more effectively
- Adding input validation
- Making the functions more maintainable
+const FILTER_OPERATORS = ['equals', 'not_equals', 'greater_than', 'less_than', 'contains'] as const; +const FILTER_RULES = ['all', 'any'] as const; + +type FilterOperator = typeof FILTER_OPERATORS[number]; +type FilterRule = typeof FILTER_RULES[number]; + export function isFilterOperator(value: string): value is FilterOperator { - return ['equals', 'not_equals', 'greater_than', 'less_than', 'contains'].includes(value); + return FILTER_OPERATORS.includes(value as FilterOperator); } export function isFilterRule(value: string): value is FilterRule { - return value === 'all' || value === 'any'; + return FILTER_RULES.includes(value as FilterRule); }
Line range hint
55-88: Optimize template processing and improve maintainability.Consider the following improvements to the template processing logic:
- Cache the regex pattern
- Add error boundaries for invalid templates
- Improve type safety
-const placeholderRegex = /{{(.*?)}}/g; +// Cache the compiled regex pattern +const PLACEHOLDER_REGEX = /{{(.*?)}}/g; +const MAX_REPLACEMENTS = 100; // Prevent infinite loops + export function parseTemplateNested(template: string, state: object, humanReadable = quickAliases): string { + if (typeof template !== 'string') { + throw new Error('Template must be a string'); + } + let parsedTemplate = template; - const matches = Array.from(parsedTemplate.matchAll(placeholderRegex)); + let replacementCount = 0; + const matches = Array.from(parsedTemplate.matchAll(PLACEHOLDER_REGEX)); for (const match of matches) { + if (replacementCount++ > MAX_REPLACEMENTS) { + throw new Error('Maximum template replacements exceeded'); + }
90-109: Enhance helper function documentation and type safety.The
formatDisplayFromStringfunction could be improved with:
- Better JSDoc documentation
- Stronger type safety
- Input validation
/** * Handles the specific case where the MaybeNumber is encoded in a string * After parsed, the value is formatted to a human-readable string + * @param value - The string value to format + * @param hideZero - Whether to hide leading zeros in the output + * @returns A formatted string representation of the value + * @throws {Error} If the input value is invalid */ function formatDisplayFromString(value: string, hideZero = false): string { + if (typeof value !== 'string') { + throw new Error('Value must be a string'); + } + let valueInNumber: MaybeNumber = null; // the value will be a string, so we need to convert it to the Maybe type if (value !== 'null') { const parsedValue = Number(value); if (!Number.isNaN(parsedValue)) { valueInNumber = parsedValue; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
apps/server/test-db/db.jsonis excluded by none and included by nonee2e/tests/fixtures/test-db.jsonis excluded by none and included by nonepackages/types/src/api/ontime-controller/BackendResponse.type.tsis excluded by none and included by nonepackages/types/src/definitions/DataModel.type.tsis excluded by none and included by nonepackages/types/src/definitions/core/Automation.type.tsis excluded by none and included by nonepackages/types/src/definitions/core/HttpSettings.type.tsis excluded by none and included by nonepackages/types/src/definitions/core/OscSettings.type.tsis excluded by none and included by nonepackages/types/src/definitions/core/TimerLifecycle.type.tsis excluded by none and included by nonepackages/types/src/index.tsis excluded by none and included by nonepackages/types/src/utils/guards.tsis excluded by none and included by none
📒 Files selected for processing (77)
apps/client/src/common/api/automation.ts(1 hunks)apps/client/src/common/api/constants.ts(1 hunks)apps/client/src/common/api/http.ts(0 hunks)apps/client/src/common/api/osc.ts(0 hunks)apps/client/src/common/hooks-query/useAutomationSettings.ts(1 hunks)apps/client/src/common/hooks-query/useHttpSettings.ts(0 hunks)apps/client/src/common/hooks-query/useOscSettings.ts(0 hunks)apps/client/src/common/models/AutomationSettings.ts(1 hunks)apps/client/src/common/models/Http.ts(0 hunks)apps/client/src/common/models/Info.ts(0 hunks)apps/client/src/common/models/OscSettings.ts(0 hunks)apps/client/src/common/utils/__tests__/regex.test.ts(1 hunks)apps/client/src/common/utils/regex.ts(1 hunks)apps/client/src/features/app-settings/AppSettings.tsx(2 hunks)apps/client/src/features/app-settings/panel-content/PanelContent.module.scss(1 hunks)apps/client/src/features/app-settings/panel-utils/PanelUtils.module.scss(3 hunks)apps/client/src/features/app-settings/panel-utils/PanelUtils.tsx(3 hunks)apps/client/src/features/app-settings/panel/automations-panel/AutomationForm.tsx(1 hunks)apps/client/src/features/app-settings/panel/automations-panel/AutomationPanel.tsx(1 hunks)apps/client/src/features/app-settings/panel/automations-panel/AutomationSettingsForm.tsx(1 hunks)apps/client/src/features/app-settings/panel/automations-panel/AutomationsList.tsx(1 hunks)apps/client/src/features/app-settings/panel/automations-panel/AutomationsListItem.tsx(1 hunks)apps/client/src/features/app-settings/panel/automations-panel/BlueprintForm.module.scss(1 hunks)apps/client/src/features/app-settings/panel/automations-panel/BlueprintForm.tsx(1 hunks)apps/client/src/features/app-settings/panel/automations-panel/BlueprintsList.tsx(1 hunks)apps/client/src/features/app-settings/panel/automations-panel/__tests__/automationUtils.test.ts(1 hunks)apps/client/src/features/app-settings/panel/automations-panel/automationUtils.ts(1 hunks)apps/client/src/features/app-settings/panel/integrations-panel/HttpIntegrations.tsx(0 hunks)apps/client/src/features/app-settings/panel/integrations-panel/IntegrationsPanel.module.css(0 hunks)apps/client/src/features/app-settings/panel/integrations-panel/IntegrationsPanel.tsx(0 hunks)apps/client/src/features/app-settings/panel/integrations-panel/OscIntegrations.tsx(0 hunks)apps/client/src/features/app-settings/panel/integrations-panel/integrationUtils.ts(0 hunks)apps/client/src/features/app-settings/panel/project-panel/ProjectMergeForm.tsx(3 hunks)apps/client/src/features/app-settings/useAppSettingsMenu.tsx(1 hunks)apps/client/src/theme/ontimeRadio.ts(1 hunks)apps/client/src/theme/theme.ts(2 hunks)apps/server/src/api-data/automation/__tests__/automation.dao.test.ts(1 hunks)apps/server/src/api-data/automation/__tests__/automation.service.test.ts(1 hunks)apps/server/src/api-data/automation/__tests__/automation.utils.test.ts(2 hunks)apps/server/src/api-data/automation/__tests__/testUtils.ts(1 hunks)apps/server/src/api-data/automation/automation.controller.ts(1 hunks)apps/server/src/api-data/automation/automation.dao.ts(1 hunks)apps/server/src/api-data/automation/automation.parser.ts(1 hunks)apps/server/src/api-data/automation/automation.router.ts(1 hunks)apps/server/src/api-data/automation/automation.service.ts(1 hunks)apps/server/src/api-data/automation/automation.utils.ts(3 hunks)apps/server/src/api-data/automation/automation.validation.ts(1 hunks)apps/server/src/api-data/automation/clients/http.client.ts(1 hunks)apps/server/src/api-data/automation/clients/osc.client.ts(1 hunks)apps/server/src/api-data/db/db.controller.ts(1 hunks)apps/server/src/api-data/http/http.controller.ts(0 hunks)apps/server/src/api-data/http/http.router.ts(0 hunks)apps/server/src/api-data/http/http.validation.ts(0 hunks)apps/server/src/api-data/index.ts(2 hunks)apps/server/src/api-data/osc/osc.controller.ts(0 hunks)apps/server/src/api-data/osc/osc.router.ts(0 hunks)apps/server/src/api-data/osc/osc.validation.ts(0 hunks)apps/server/src/api-data/session/session.service.ts(0 hunks)apps/server/src/app.ts(2 hunks)apps/server/src/classes/data-provider/DataProvider.ts(4 hunks)apps/server/src/classes/data-provider/DataProvider.utils.ts(2 hunks)apps/server/src/classes/data-provider/__tests__/DataProvider.utils.test.ts(2 hunks)apps/server/src/models/dataModel.ts(1 hunks)apps/server/src/models/demoProject.ts(1 hunks)apps/server/src/services/integration-service/HttpIntegration.ts(0 hunks)apps/server/src/services/integration-service/IIntegration.ts(0 hunks)apps/server/src/services/integration-service/IntegrationService.ts(0 hunks)apps/server/src/services/integration-service/OscIntegration.ts(0 hunks)apps/server/src/services/project-service/ProjectService.ts(2 hunks)apps/server/src/services/runtime-service/RuntimeService.ts(11 hunks)apps/server/src/stores/EventStore.ts(1 hunks)apps/server/src/stores/__mocks__/runtimeState.mocks.ts(1 hunks)apps/server/src/utils/__tests__/parser.test.ts(1 hunks)apps/server/src/utils/__tests__/parserFunctions.test.ts(0 hunks)apps/server/src/utils/assert.ts(1 hunks)apps/server/src/utils/parser.ts(2 hunks)apps/server/src/utils/parserFunctions.ts(1 hunks)
💤 Files with no reviewable changes (24)
- apps/client/src/common/models/Info.ts
- apps/client/src/common/models/Http.ts
- apps/server/src/api-data/osc/osc.router.ts
- apps/server/src/api-data/http/http.router.ts
- apps/client/src/common/models/OscSettings.ts
- apps/client/src/features/app-settings/panel/integrations-panel/IntegrationsPanel.tsx
- apps/server/src/api-data/http/http.validation.ts
- apps/client/src/features/app-settings/panel/integrations-panel/integrationUtils.ts
- apps/server/src/api-data/session/session.service.ts
- apps/server/src/services/integration-service/IntegrationService.ts
- apps/client/src/features/app-settings/panel/integrations-panel/OscIntegrations.tsx
- apps/server/src/services/integration-service/IIntegration.ts
- apps/client/src/common/api/osc.ts
- apps/server/src/services/integration-service/OscIntegration.ts
- apps/server/src/api-data/osc/osc.validation.ts
- apps/client/src/features/app-settings/panel/integrations-panel/HttpIntegrations.tsx
- apps/client/src/common/api/http.ts
- apps/client/src/features/app-settings/panel/integrations-panel/IntegrationsPanel.module.css
- apps/server/src/services/integration-service/HttpIntegration.ts
- apps/server/src/api-data/osc/osc.controller.ts
- apps/server/src/api-data/http/http.controller.ts
- apps/server/src/utils/tests/parserFunctions.test.ts
- apps/client/src/common/hooks-query/useHttpSettings.ts
- apps/client/src/common/hooks-query/useOscSettings.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/server/src/api-data/automation/__tests__/automation.service.test.ts
[error] 27-27: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
apps/server/src/api-data/automation/__tests__/automation.dao.test.ts
[error] 31-31: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
apps/server/src/api-data/automation/automation.controller.ts
[error] 59-59: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 99-99: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 109-109: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (37)
apps/server/src/services/project-service/ProjectService.ts (3)
177-177: LGTM! Clean separation of concerns.The simplified destructuring aligns with the PR's objective of moving integration logic to the new automation service.
247-247: LGTM! Consistent with the changes in loadProjectFile.The same simplified destructuring is correctly applied here, maintaining consistency across the codebase.
Line range hint
1-300: Verify complete removal of integration-related code.Let's ensure no integration-related code or references remain in the project service.
✅ Verification successful
Integration-related code has been successfully removed
The project service is now focused solely on project file management operations, with no remaining traces of integration-related code. The only external dependencies are for core functionality like runtime and app state management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining integration-related code in the project service # Check for integration-related imports and usage rg -i "integration|osc|http" "apps/server/src/services/project-service" # Check for any remaining integration service references ast-grep --pattern 'integration' "apps/server/src/services/project-service"Length of output: 145
Script:
#!/bin/bash # Broader search for integration-related terms and configurations rg -i "midi|external|protocol|connector|bridge" "apps/server/src/services/project-service" # Check imported services and their dependencies ast-grep --pattern 'import { $_ } from "../$_-service/$_"' # Look for any remaining configuration or settings rg -i "config\.|settings\." "apps/server/src/services/project-service"Length of output: 628
apps/server/src/api-data/index.ts (2)
Line range hint
3-13: LGTM! Verify removal of HTTP and OSC routers.The addition of the automations router follows the established naming and import patterns.
Let's verify the complete removal of HTTP and OSC related files:
✅ Verification successful
HTTP and OSC routers have been completely removed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining HTTP/OSC integration files # Expected: No matches as these should be completely removed echo "Checking for HTTP integration files..." fd -e ts -e js "http\.(router|controller|validation|service)" apps/server/src echo "Checking for OSC integration files..." fd -e ts -e js "osc\.(router|controller|validation|service)" apps/server/srcLength of output: 330
Script:
#!/bin/bash # Perform a broader search for any HTTP/OSC related code echo "Checking for HTTP-related code..." rg -i "http(router|controller|handler|middleware|config)" apps/server/src echo "Checking for OSC-related code..." rg -i "osc(router|controller|handler|middleware|config)" apps/server/src echo "Checking for specific imports..." rg -i "import.*['\"].*/(http|osc)/" apps/server/srcLength of output: 433
Line range hint
17-27: LGTM! Consider documenting the architectural changes.The addition of the
/automationsendpoint and removal of HTTP/OSC integrations represent a significant architectural shift. Consider updating the API documentation to reflect these changes.Let's check for existing API documentation that might need updates:
apps/server/src/api-data/automation/clients/http.client.ts (1)
1-7: LGTM! Imports are well-organized and properly typed.apps/server/src/classes/data-provider/__tests__/DataProvider.utils.test.ts (2)
38-43: LGTM! The automation configuration aligns with the PR objectives.The new automation configuration structure correctly reflects the transition from integrations to automations, with appropriate default values for the settings.
Line range hint
38-131: Verify the automation model structure.Let's verify that the automation configuration structure matches the actual model definition.
✅ Verification successful
Automation model structure verified successfully
The automation configuration structure in the test file exactly matches the
AutomationSettingstype definition from the core types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the automation type definition to verify the structure ast-grep --pattern 'type $_ = { enabledAutomations: $_, enabledOscIn: $_, oscPortIn: $_, automations: $_, blueprints: $_, }'Length of output: 136
Script:
#!/bin/bash # Find automation-related files fd -t f automation # Search for automation type/interface definitions with broader pattern ast-grep --pattern 'interface Automation { $$$ }' ast-grep --pattern 'type Automation = { $$$ }' # Fallback to ripgrep to find automation type definitions rg -l "type.*Automation.*=|interface.*Automation.*{" --type tsLength of output: 2253
Script:
#!/bin/bash # Check the content of the automation type definitions file cat packages/types/src/definitions/core/Automation.type.ts # Search for type containing enabledAutomations rg "enabledAutomations" --type ts -C 5Length of output: 21656
apps/server/src/utils/parserFunctions.ts (2)
18-18: LGTM! Import statements are well-organized.The consolidation of imports and removal of unused OSC/HTTP-related imports aligns well with the PR's objective of rewriting the integrations service.
Also applies to: 22-22
Line range hint
1-285: Verify the complete removal of OSC/HTTP parsing references.The removal of OSC and HTTP parsing functions appears to be part of the larger restructuring effort. Let's verify that all references to these removed functions have been properly handled.
✅ Verification successful
Removal of OSC/HTTP parsing functions verified successfully
The remaining OSC/HTTP related files are part of the new implementation and don't depend on the removed parsing functions. The codebase shows a clean separation of concerns with the OSC/HTTP functionality properly isolated in the automation system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to the removed OSC/HTTP parsing functions # Search for references to removed functions echo "Searching for references to removed parsing functions..." rg -l "sanitiseOscSubscriptions|parseOsc|sanitiseHttpSubscriptions|parseHttp" # Search for any remaining OSC/HTTP types usage echo "Searching for OSC/HTTP types usage..." rg -l "OscSubscription|HttpSubscription|OSCSettings|HttpSettings"Length of output: 355
Script:
#!/bin/bash # Broader search for OSC/HTTP related code # Search for OSC/HTTP related files echo "Searching for OSC/HTTP related files..." fd -t f -e ts -e js "osc|http" --ignore-case # Search for OSC/HTTP related imports and configurations echo "Searching for OSC/HTTP related code..." rg -i "(?i)(osc|http).*\b(settings?|subscriptions?|parse|config)\b" -g "*.{ts,js}" # Search for any remaining imports from removed modules echo "Searching for imports..." ast-grep --pattern 'import { $$$ } from $path' | grep -i "osc\|http"Length of output: 1756
Script:
#!/bin/bash # Check content of OSC/HTTP related files echo "Checking OSC adapter content..." cat "apps/server/src/adapters/OscAdapter.ts" echo -e "\nChecking OSC/HTTP clients content..." cat "apps/server/src/api-data/automation/clients/osc.client.ts" cat "apps/server/src/api-data/automation/clients/http.client.ts" echo -e "\nChecking OSC parser content..." cat "apps/server/src/utils/oscArgParser.ts"Length of output: 6334
apps/client/src/features/app-settings/panel-utils/PanelUtils.module.scss (3)
29-30: LGTM! Good use of shared styles.The grouping of
.sectionand.indentclasses is clean and follows DRY principles.
186-186: LGTM! Consistent spacing.The padding adjustment aligns well with the spacing system used throughout the styles.
115-119: Verify color contrast for warning state.While using data attributes for styling is a good practice, please ensure that the
$orange-1300background color provides sufficient contrast with the text color for accessibility compliance.Run this script to check if there are any color contrast related variables or utilities in the codebase:
✅ Verification successful
Color contrast is appropriate for warning state
The dark orange background (#3D2900) with light text follows accessibility best practices and provides sufficient contrast for warning states in the table.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for color contrast utilities or documentation rg -i "contrast|a11y|accessibility" --type scssLength of output: 78
Script:
#!/bin/bash # Find all SCSS and CSS files echo "=== Finding SCSS and CSS files ===" fd -e scss -e css # Search for orange color variables and related documentation echo -e "\n=== Searching for orange color definitions ===" rg "\$orange-|// .*orange" -t css -t scss # Look for design system or color documentation echo -e "\n=== Searching for color system documentation ===" fd -e md -e mdx | xargs rg -i "color.*(system|palette|accessibility)"Length of output: 7496
Script:
#!/bin/bash # Check theme files for color definitions echo "=== Checking theme files for color system ===" cat "apps/client/src/theme/_ontimeColours.scss" # Search for orange color usage across files echo -e "\n=== Searching for orange color usage ===" rg -l "\$orange" $(fd -e scss) # Check the specific file for context echo -e "\n=== Checking PanelUtils.module.scss context ===" cat "apps/client/src/features/app-settings/panel-utils/PanelUtils.module.scss"Length of output: 13286
apps/server/src/app.ts (2)
15-15: [Approved] Necessary imports are correctly included.The imports from
./externals.jsare properly adjusted to reflect the changes in the application's architecture. All imported variables (environment,isProduction,updateRouterPrefix) are used appropriately in the code.
132-132: [Approved] Simplified error message enhances clarity.The updated error message in the
checkStartfunction is concise and focuses on the essential initialization steps, improving readability.apps/server/src/api-data/automation/automation.service.ts (1)
111-113: Verify that OSC outputs are correctly skipped in Ontime Cloud environments.In the
sendfunction, OSC outputs are conditionally emitted based on theisOntimeCloudflag:if (!isOntimeCloud) { emitOSC(payload, stateSnapshot); }Please confirm that this logic aligns with the intended behavior, ensuring that OSC outputs are appropriately handled or skipped when running in Ontime Cloud environments.
✅ Verification successful
OSC outputs are correctly skipped in Ontime Cloud environments
The verification confirms that:
- The
isOntimeCloudflag is properly set via environment variables on the server- The OSC emission is consistently guarded by this flag
- There are no other OSC emission points that could bypass this check
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find the complete send function implementation ast-grep --pattern 'send($$$) { $$$ }' # Search for isOntimeCloud definition and usage rg "isOntimeCloud" -A 2 # Find all emitOSC calls rg "emitOSC\(" -A 2 # Look for Ontime Cloud configuration rg -i "ontime.*cloud" -A 2Length of output: 6475
apps/client/src/common/models/AutomationSettings.ts (1)
5-6: Consider relocating OSC input settings to app settings.Based on previous discussions, OSC input settings such as
enabledOscInandoscPortInmight be better placed within app settings rather than withinautomationPlaceholderSettings, to maintain separation of concerns between automation configurations and OSC input configurations.apps/client/src/common/utils/regex.ts (1)
8-8: LGTM! Good improvement to support HTTPS.The updated regex pattern now correctly handles both HTTP and HTTPS protocols, which is essential for secure connections in the automation service.
apps/client/src/features/app-settings/panel/automations-panel/__tests__/automationUtils.test.ts (1)
16-22: Verify the expected result in the duplicate test case.The test data shows two entries with title 'Third' (lines 19-20), but the assertion expects [2]. This seems inconsistent with the apparent duplicate entries. Please verify if:
- The implementation considers different triggers as unique entries
- The test data accurately represents the intended duplicate scenario
- The expected result [2] is correct
apps/client/src/common/hooks-query/useAutomationSettings.ts (1)
24-34: LGTM! Well-structured mutation hook.The mutation implementation follows best practices with proper error handling, optimistic updates via
setQueryData, and cache invalidation.apps/client/src/features/app-settings/AppSettings.tsx (1)
6-6: LGTM! Clean migration from integrations to automation panel.The changes align well with the PR's objective of renaming integrations to automation, maintaining a clean component structure.
Also applies to: 33-33
apps/client/src/features/app-settings/useAppSettingsMenu.tsx (1)
50-55: LGTM! Well-structured menu options for automation features.The new automation menu options provide clear navigation paths while maintaining consistency with the existing menu structure.
apps/client/src/features/app-settings/panel/project-panel/ProjectMergeForm.tsx (1)
26-26: LGTM! Clean transition to automation settings.The changes correctly implement the transition from OSC/HTTP settings to the new automation system while maintaining the existing form structure and validation logic.
Also applies to: 44-44, 116-117
apps/client/src/theme/theme.ts (1)
17-17: LGTM! Proper theme configuration.The new radio variant is correctly implemented following Chakra UI's theming patterns.
Also applies to: 112-112
apps/server/src/classes/data-provider/DataProvider.ts (1)
121-128: LGTM! Consistent implementation pattern.The new automation settings functions follow the established pattern of other settings management functions, maintaining type safety and immutability.
apps/client/src/features/app-settings/panel-utils/PanelUtils.tsx (1)
60-60: LGTM!The className composition using the cx utility is implemented correctly.
apps/client/src/features/app-settings/panel/automations-panel/BlueprintsList.tsx (1)
1-24: LGTM! Clean imports and well-defined props interface.The component's dependencies and type definitions are properly structured.
apps/server/src/api-data/automation/__tests__/automation.utils.test.ts (1)
1-1: LGTM! Import statement updated correctly.The import statement has been properly updated to use the local automation utils module.
apps/server/src/api-data/db/db.controller.ts (1)
21-30: LGTM! Property replacement aligns with architecture changes.The replacement of
httpwithautomationin the request body destructuring andpatchDbobject aligns with the PR's objective of transitioning from integrations to automation.apps/server/src/models/demoProject.ts (1)
453-458: Verify default automation settings.The default settings look reasonable, but please verify:
- Is
enabledAutomations: falsethe intended default for demo projects?- Should
oscPortIn: 8888be configurable rather than hardcoded?Run this script to check for other occurrences of the port number:
✅ Verification successful
Default automation settings are correct and port configuration is intentional
The verification confirms:
enabledAutomations: falseis the intended safe default for demo projects- Port 8888 is standardized for OSC input across the application and explicitly configured in Docker setup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for hardcoded port 8888 rg -l "8888"Length of output: 506
Script:
#!/bin/bash # Check for environment variables or configuration related to port 8888 rg -A 5 "port.*8888|8888.*port" --type ts --type yml --type env # Check AutomationSettings.ts content cat apps/client/src/common/models/AutomationSettings.ts # Look for demo project initialization patterns rg -A 5 "demo.*project.*default|default.*demo.*project" --type tsLength of output: 449
Script:
#!/bin/bash # Check docker and yaml files for port configuration rg -A 5 "port.*8888|8888.*port" docker-compose.yml Dockerfile # Look for environment variable patterns for port configuration rg -A 5 "PORT|OSC_PORT|AUTOMATION_PORT" --type ts --type env # Check for any configuration files fd -e yaml -e yml -e json -e configLength of output: 628
Script:
#!/bin/bash # Check test db and server package.json for configuration cat apps/server/test-db/db.json cat apps/server/package.json # Check docker-compose for port configuration cat docker-compose.yml # One final check for environment variable patterns in TypeScript files rg "process\.env\." --type ts -A 2Length of output: 14245
apps/server/src/utils/parser.ts (2)
25-28: LGTM! Import changes align with the automation service refactor.The changes correctly reflect the transition from integrations to automation by:
- Removing HTTP and OSC imports
- Adding automation settings parser import
33-33: LGTM! Well-defined error handling type.The
ErrorEmittertype enhances error handling capabilities by providing a clear contract for error reporting functions.apps/server/src/services/runtime-service/RuntimeService.ts (1)
78-82: LGTM! Consistent migration from integrations to automations.The changes systematically replace
integrationService.dispatchwithtriggerAutomationsacross all timer lifecycle events:
- Warning phase
- Danger phase
- Timer finish
- Timer update
- Clock update
- Event load
- Timer start
- Timer pause
- Timer stop
- Timer roll
The migration maintains the same event handling flow while transitioning to the new automation system.
Also applies to: 98-98, 118-118, 138-138, 148-148, 295-295, 474-474, 527-527, 546-546, 594-604
apps/server/src/utils/__tests__/parser.test.ts (1)
317-317: Verify the reduction in expected error count.The test now expects 5 errors instead of 7 for duplicate IDs. Please confirm if this change is intentional and aligns with the new error handling logic in the parser.
✅ Verification successful
The error count change is intentional and correct
The test case "detects duplicate Ids" is specifically testing imports outside the domain, and the change from 7 to 5 errors is correct. The test intentionally provides malformed data with duplicate IDs and missing required sections, and the parser correctly identifies 5 validation errors across different contexts (Rundown, Project, View Settings, etc.).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for related changes in error handling logic # Search for error-related code in the parser rg -A 5 "errors\.push|makeEmitError" apps/server/src/utils/parser.tsLength of output: 865
Script:
#!/bin/bash # Search for duplicate ID related code and validation rg -B 2 -A 5 "duplicate.*id" apps/server/src/utils/parser.ts # Get the complete test case context rg -B 10 -A 10 "expect\(errors\.length\)\.toBe\(5\)" apps/server/src/utils/__tests__/parser.test.tsLength of output: 705
Script:
#!/bin/bash # Get the complete test case ast-grep --pattern 'it($_, () => { $$$ expect(errors.length).toBe(5); $$$ })' apps/server/src/utils/__tests__/parser.test.ts # Search for validation rules in parser rg -B 5 -A 5 "validate|check" apps/server/src/utils/parser.tsLength of output: 5267
Script:
#!/bin/bash # Search for required fields and their validation rg -B 5 -A 5 "required" apps/server/src/utils/parser.ts # Get the test file's required settings rg -B 2 -A 2 "requiredSettings" apps/server/src/utils/__tests__/parser.test.tsLength of output: 783
Script:
#!/bin/bash # Get the parser implementation focusing on validation and error handling ast-grep --pattern 'function parseDatabaseModel($_) { $$$ }' apps/server/src/utils/parser.ts # Search for any error messages in the parser rg "Error.*:|error.*:" apps/server/src/utils/parser.tsLength of output: 508
apps/client/src/features/app-settings/panel-content/PanelContent.module.scss (1)
20-20: Verify the large padding value.A padding-bottom of 300px seems unusually large. Please confirm if this size is necessary to accommodate the new automation UI components without causing content overlap.
✅ Verification successful
The 300px bottom padding is appropriate for this component
The padding is justified as the component renders multiple different panels (Automation, Sources, Network, etc.) with varying content heights in a scrollable container. This extra space ensures content remains accessible and prevents awkward cutoffs at the bottom of the scrollable area.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for related UI components that might require this padding # Search for components using this class rg -A 5 "PanelContent_content" apps/client/src/features/app-settings/Length of output: 69
Script:
#!/bin/bash # Find the module file and its references echo "=== Searching for PanelContent.module.scss ===" fd "PanelContent.module.scss" apps/client/src/ echo -e "\n=== Finding references to PanelContent component ===" rg -l "PanelContent" apps/client/src/features/app-settings/ echo -e "\n=== Checking content of PanelContent component ===" rg -A 10 "class.*PanelContent" apps/client/src/features/app-settings/Length of output: 719
Script:
#!/bin/bash echo "=== PanelContent.tsx Implementation ===" cat apps/client/src/features/app-settings/panel-content/PanelContent.tsx echo -e "\n=== AppSettings.tsx Usage ===" rg -A 5 -B 5 "PanelContent" apps/client/src/features/app-settings/AppSettings.tsx echo -e "\n=== Checking for scroll/overflow styles ===" rg "overflow|scroll" apps/client/src/features/app-settings/panel-content/PanelContent.module.scssLength of output: 2661
apps/client/src/features/app-settings/panel/automations-panel/BlueprintForm.module.scss (1)
1-14: LGTM! Well-structured layout classes.The
.outerColumnand.innerColumnclasses provide a clean and consistent layout structure with appropriate spacing.apps/client/src/common/utils/__tests__/regex.test.ts (1)
29-30: Verify regex implementation matches test requirements.Let's verify that the regex implementation in the codebase properly handles both HTTP and HTTPS URLs.
✅ Verification successful
Regex implementation correctly validates HTTP/HTTPS URLs
The regex pattern
/^https?:\/\//properly matches the test cases, validating URLs that start with either "http://" or "https://" while rejecting invalid formats.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find and verify the regex implementation for URL validation # Search for the regex pattern definition echo "Searching for startsWithHttp regex definition:" rg -l "startsWithHttp.*=" src/Length of output: 180
Script:
#!/bin/bash # Search for regex implementation and tests echo "Searching for regex files:" fd regex "apps/client/src" echo -e "\nChecking content of relevant files:" for f in $(fd regex "apps/client/src"); do echo -e "\n=== Content of $f ===" cat "$f" doneLength of output: 3252
| /** Emits message over transport */ | ||
| async function emit(url: string) { | ||
| logger.info(LogOrigin.Rx, `Sending HTTP: ${url}`); | ||
| try { | ||
| const response = await fetch(url); | ||
| if (!response.ok) { | ||
| if (response.status >= 500 && response.status < 600) { | ||
| logger.warning(LogOrigin.Tx, `HTTP Integration: Server refused message ${response.status}`); | ||
| } else if (response.status >= 400) { | ||
| logger.warning(LogOrigin.Tx, `HTTP Integration: Failed sending message ${response.status}`); | ||
| } else { | ||
| logger.warning(LogOrigin.Tx, `HTTP Integration: Failed sending message ${response.status}`); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| if (!(error instanceof Error)) { | ||
| logger.warning(LogOrigin.Tx, `HTTP Integration: Failed sending message ${error}`); | ||
| return; | ||
| } | ||
|
|
||
| logger.warning(LogOrigin.Tx, `HTTP Integration: ${error.name} ${error.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance HTTP client robustness and error handling.
Several improvements could make the HTTP client more robust:
- The else clause (lines 32-34) duplicates the 400-level error message
- Missing timeout handling for fetch requests
- Could benefit from response body logging for debugging
- No retry mechanism for transient failures
- Missing abort controller for request cancellation
-async function emit(url: string) {
+async function emit(url: string, options = { timeout: 5000, retries: 3 }) {
logger.info(LogOrigin.Rx, `Sending HTTP: ${url}`);
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), options.timeout);
+
+ let attempts = 0;
+ while (attempts < options.retries) {
try {
- const response = await fetch(url);
+ const response = await fetch(url, { signal: controller.signal });
+ clearTimeout(timeoutId);
+
if (!response.ok) {
+ const responseText = await response.text();
+ logger.debug(LogOrigin.Tx, `Response body: ${responseText}`);
+
if (response.status >= 500 && response.status < 600) {
logger.warning(LogOrigin.Tx, `HTTP Integration: Server refused message ${response.status}`);
+ // Retry server errors
+ attempts++;
+ if (attempts < options.retries) {
+ await new Promise(resolve => setTimeout(resolve, 1000 * attempts));
+ continue;
+ }
} else if (response.status >= 400) {
logger.warning(LogOrigin.Tx, `HTTP Integration: Failed sending message ${response.status}`);
- } else {
- logger.warning(LogOrigin.Tx, `HTTP Integration: Failed sending message ${response.status}`);
}
}
+ break;
} catch (error) {
+ clearTimeout(timeoutId);
+
if (!(error instanceof Error)) {
logger.warning(LogOrigin.Tx, `HTTP Integration: Failed sending message ${error}`);
return;
}
+ if (error.name === 'AbortError') {
+ logger.warning(LogOrigin.Tx, `HTTP Integration: Request timed out after ${options.timeout}ms`);
+ return;
+ }
+
logger.warning(LogOrigin.Tx, `HTTP Integration: ${error.name} ${error.message}`);
+
+ // Retry network errors
+ attempts++;
+ if (attempts < options.retries) {
+ await new Promise(resolve => setTimeout(resolve, 1000 * attempts));
+ continue;
+ }
+ break;
}
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Emits message over transport */ | |
| async function emit(url: string) { | |
| logger.info(LogOrigin.Rx, `Sending HTTP: ${url}`); | |
| try { | |
| const response = await fetch(url); | |
| if (!response.ok) { | |
| if (response.status >= 500 && response.status < 600) { | |
| logger.warning(LogOrigin.Tx, `HTTP Integration: Server refused message ${response.status}`); | |
| } else if (response.status >= 400) { | |
| logger.warning(LogOrigin.Tx, `HTTP Integration: Failed sending message ${response.status}`); | |
| } else { | |
| logger.warning(LogOrigin.Tx, `HTTP Integration: Failed sending message ${response.status}`); | |
| } | |
| } | |
| } catch (error) { | |
| if (!(error instanceof Error)) { | |
| logger.warning(LogOrigin.Tx, `HTTP Integration: Failed sending message ${error}`); | |
| return; | |
| } | |
| logger.warning(LogOrigin.Tx, `HTTP Integration: ${error.name} ${error.message}`); | |
| } | |
| } | |
| /** Emits message over transport */ | |
| async function emit(url: string, options = { timeout: 5000, retries: 3 }) { | |
| logger.info(LogOrigin.Rx, `Sending HTTP: ${url}`); | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), options.timeout); | |
| let attempts = 0; | |
| while (attempts < options.retries) { | |
| try { | |
| const response = await fetch(url, { signal: controller.signal }); | |
| clearTimeout(timeoutId); | |
| if (!response.ok) { | |
| const responseText = await response.text(); | |
| logger.debug(LogOrigin.Tx, `Response body: ${responseText}`); | |
| if (response.status >= 500 && response.status < 600) { | |
| logger.warning(LogOrigin.Tx, `HTTP Integration: Server refused message ${response.status}`); | |
| // Retry server errors | |
| attempts++; | |
| if (attempts < options.retries) { | |
| await new Promise(resolve => setTimeout(resolve, 1000 * attempts)); | |
| continue; | |
| } | |
| } else if (response.status >= 400) { | |
| logger.warning(LogOrigin.Tx, `HTTP Integration: Failed sending message ${response.status}`); | |
| } | |
| } | |
| break; | |
| } catch (error) { | |
| clearTimeout(timeoutId); | |
| if (!(error instanceof Error)) { | |
| logger.warning(LogOrigin.Tx, `HTTP Integration: Failed sending message ${error}`); | |
| return; | |
| } | |
| if (error.name === 'AbortError') { | |
| logger.warning(LogOrigin.Tx, `HTTP Integration: Request timed out after ${options.timeout}ms`); | |
| return; | |
| } | |
| logger.warning(LogOrigin.Tx, `HTTP Integration: ${error.name} ${error.message}`); | |
| // Retry network errors | |
| attempts++; | |
| if (attempts < options.retries) { | |
| await new Promise(resolve => setTimeout(resolve, 1000 * attempts)); | |
| continue; | |
| } | |
| break; | |
| } | |
| } | |
| } |
apps/server/src/api-data/automation/__tests__/automation.dao.test.ts
Outdated
Show resolved
Hide resolved
apps/server/src/api-data/automation/__tests__/automation.service.test.ts
Show resolved
Hide resolved
apps/server/src/api-data/automation/__tests__/automation.service.test.ts
Outdated
Show resolved
Hide resolved
apps/client/src/features/app-settings/panel/automations-panel/AutomationForm.tsx
Outdated
Show resolved
Hide resolved
cpvalente
force-pushed
the
integrations
branch
4 times, most recently
from
a5663da to
97d34bf
Compare
|
@alex-Arc , I believe this is ready to merge, would you mind sanity checking? |
cpvalente
force-pushed
the
integrations
branch
from
97d34bf to
f9b9e64
Compare
| @@ -26,8 +26,8 @@ describe('simple tests for regex', () => { | |||
| }); | |||
|
|
|||
| test('startsWithHttp', () => { | |||
| const right = ['http://test']; | |||
| const wrong = ['https://test', 'testing', '123.0.1']; | |||
| const right = ['https://test', 'http://test']; | |||
There was a problem hiding this comment.
Did you get https working?
or did it always work we just did not allow it
There was a problem hiding this comment.
i have been testing with ontime cloud and it seems fine. can you verify in one of your containers?
| @@ -283,7 +257,6 @@ export const shutdown = async (exitCode = 0) => { | |||
|
|
|||
| expressServer?.close(); | |||
| runtimeService.shutdown(); | |||
There was a problem hiding this comment.
do we not still need the osc shutdown here?
cpvalente
force-pushed
the
integrations
branch
from
f9b9e64 to
b732178
Compare
merge with automation service
rename automations > triggers rename blueprints > automations chore: add link to documentation
cpvalente
force-pushed
the
integrations
branch
from
a90c10f to
3854534
Compare
This is an attempt at re-writing the integrations service
For simplicity, I am renaming this to automation, this means that integrations becomes an internal name used for data into ontime
There are also some dependency upgrades which make the PR larger
The code parsing logic is the same as before, so hopefully there are not that many core changes to the outputs, however, the filtering logic is completely new
I am also proposing a new structure to our backend, it is annoying that I only did it in automations and - if we decide to go forwards with this - I will create a follow up PR to apply everywhere
daofileautomationdirectory, contains all logic related to that domain conceptcontrollerandrouterfiles, but didnt see an obvious improvement. in a way, it is nice to be able to read all the available routes in a small file without worrying about what they doIn this PR
create migration code for old subscriptions(decided to migrate OSC data and leave the rest)TODO