From 582b0154aca7c675573711f1316c903135003307 Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Wed, 11 Dec 2024 12:44:40 -0500 Subject: [PATCH 01/11] refactor --- .../input/AssetSelectionInput.oss.tsx | 276 +++++------------- .../src/selection/SelectionAutoComplete.ts | 16 +- .../selection/SelectionAutoCompleteInput.tsx | 210 +++++++++++++ .../__tests__/SelectionAutoComplete.test.ts | 71 ++++- 4 files changed, 354 insertions(+), 219 deletions(-) create mode 100644 js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx diff --git a/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionInput.oss.tsx b/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionInput.oss.tsx index f3060d7f460f8..24e10e52622a1 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionInput.oss.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionInput.oss.tsx @@ -1,17 +1,11 @@ -import {Colors, Icon, Icons} from '@dagster-io/ui-components'; -import CodeMirror, {Editor, HintFunction} from 'codemirror'; -import {useLayoutEffect, useMemo, useRef} from 'react'; -import styled, {createGlobalStyle, css} from 'styled-components'; +import {Icons} from '@dagster-io/ui-components'; +import {useMemo} from 'react'; +import styled from 'styled-components'; import {lintAssetSelection} from './AssetSelectionLinter'; import {assertUnreachable} from '../../app/Util'; import {AssetGraphQueryItem} from '../../asset-graph/useAssetGraphData'; -import {useUpdatingRef} from '../../hooks/useUpdatingRef'; -import {createSelectionHint} from '../../selection/SelectionAutoComplete'; -import { - SelectionAutoCompleteInputCSS, - applyStaticSyntaxHighlighting, -} from '../../selection/SelectionAutoCompleteHighlighter'; +import {SelectionAutoCompleteInput, iconStyle} from '../../selection/SelectionAutoCompleteInput'; import {placeholderTextForItems} from '../../ui/GraphQueryInput'; import {buildRepoPathForHuman} from '../../workspace/buildRepoAddress'; @@ -32,215 +26,81 @@ interface AssetSelectionInputProps { const FUNCTIONS = ['sinks', 'roots']; export const AssetSelectionInput = ({value, onChange, assets}: AssetSelectionInputProps) => { - const editorRef = useRef(null); - const cmInstance = useRef(null); - - const currentValueRef = useRef(value); - - const hintRef = useUpdatingRef( - useMemo(() => { - const assetNamesSet: Set = new Set(); - const tagNamesSet: Set = new Set(); - const ownersSet: Set = new Set(); - const groupsSet: Set = new Set(); - const kindsSet: Set = new Set(); - const codeLocationSet: Set = new Set(); - - assets.forEach((asset) => { - assetNamesSet.add(asset.name); - asset.node.tags.forEach((tag) => { - if (tag.key && tag.value) { - tagNamesSet.add(`${tag.key}=${tag.value}`); - } else { - tagNamesSet.add(tag.key); - } - }); - asset.node.owners.forEach((owner) => { - switch (owner.__typename) { - case 'TeamAssetOwner': - ownersSet.add(owner.team); - break; - case 'UserAssetOwner': - ownersSet.add(owner.email); - break; - default: - assertUnreachable(owner); - } - }); - if (asset.node.groupName) { - groupsSet.add(asset.node.groupName); - } - asset.node.kinds.forEach((kind) => { - kindsSet.add(kind); - }); - const location = buildRepoPathForHuman( - asset.node.repository.name, - asset.node.repository.location.name, - ); - codeLocationSet.add(location); - }); - - const assetNames = Array.from(assetNamesSet); - const tagNames = Array.from(tagNamesSet); - const owners = Array.from(ownersSet); - const groups = Array.from(groupsSet); - const kinds = Array.from(kindsSet); - const codeLocations = Array.from(codeLocationSet); - - return createSelectionHint( - 'key', - { - key: assetNames, - tag: tagNames, - owner: owners, - group: groups, - kind: kinds, - code_location: codeLocations, - }, - FUNCTIONS, - ); - }, [assets]), - ); - - useLayoutEffect(() => { - if (editorRef.current && !cmInstance.current) { - cmInstance.current = CodeMirror(editorRef.current, { - value, - mode: 'assetSelection', - lineNumbers: false, - lineWrapping: false, - scrollbarStyle: 'native', - autoCloseBrackets: true, - lint: { - getAnnotations: lintAssetSelection, - async: false, - }, - placeholder: placeholderTextForItems('Type an asset subset…', assets), - extraKeys: { - 'Ctrl-Space': 'autocomplete', - Tab: (cm: Editor) => { - cm.replaceSelection(' ', 'end'); - }, - }, - }); - - cmInstance.current.setSize('100%', 20); - - // Enforce single line by preventing newlines - cmInstance.current.on('beforeChange', (_instance: Editor, change) => { - if (change.text.some((line) => line.includes('\n'))) { - change.cancel(); + const attributesMap = useMemo(() => { + const assetNamesSet: Set = new Set(); + const tagNamesSet: Set = new Set(); + const ownersSet: Set = new Set(); + const groupsSet: Set = new Set(); + const kindsSet: Set = new Set(); + const codeLocationSet: Set = new Set(); + + assets.forEach((asset) => { + assetNamesSet.add(asset.name); + asset.node.tags.forEach((tag) => { + if (tag.key && tag.value) { + tagNamesSet.add(`${tag.key}=${tag.value}`); + } else { + tagNamesSet.add(tag.key); } }); - - cmInstance.current.on('change', (instance: Editor, change) => { - const newValue = instance.getValue().replace(/\s+/g, ' '); - currentValueRef.current = newValue; - onChange(newValue); - - if (change.origin === 'complete' && change.text[0]?.endsWith('()')) { - // Set cursor inside the right parenthesis - const cursor = instance.getCursor(); - instance.setCursor({...cursor, ch: cursor.ch - 1}); + asset.node.owners.forEach((owner) => { + switch (owner.__typename) { + case 'TeamAssetOwner': + ownersSet.add(owner.team); + break; + case 'UserAssetOwner': + ownersSet.add(owner.email); + break; + default: + assertUnreachable(owner); } }); - - cmInstance.current.on('inputRead', (instance: Editor) => { - showHint(instance, hintRef.current); - }); - - cmInstance.current.on('cursorActivity', (instance: Editor) => { - applyStaticSyntaxHighlighting(instance); - showHint(instance, hintRef.current); - }); - - requestAnimationFrame(() => { - if (!cmInstance.current) { - return; - } - - applyStaticSyntaxHighlighting(cmInstance.current); + if (asset.node.groupName) { + groupsSet.add(asset.node.groupName); + } + asset.node.kinds.forEach((kind) => { + kindsSet.add(kind); }); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); - - // Update CodeMirror when value prop changes - useLayoutEffect(() => { - const noNewLineValue = value.replace('\n', ' '); - if (cmInstance.current && cmInstance.current.getValue() !== noNewLineValue) { - const instance = cmInstance.current; - const cursor = instance.getCursor(); - instance.setValue(noNewLineValue); - instance.setCursor(cursor); - showHint(instance, hintRef.current); - } - }, [hintRef, value]); + const location = buildRepoPathForHuman( + asset.node.repository.name, + asset.node.repository.location.name, + ); + codeLocationSet.add(location); + }); + const assetNames = Array.from(assetNamesSet); + const tagNames = Array.from(tagNamesSet); + const owners = Array.from(ownersSet); + const groups = Array.from(groupsSet); + const kinds = Array.from(kindsSet); + const codeLocations = Array.from(codeLocationSet); + + return { + key: assetNames, + tag: tagNames, + owner: owners, + group: groups, + kind: kinds, + code_location: codeLocations, + }; + }, [assets]); return ( - <> - - - -
- - - + + + ); }; -const iconStyle = (img: string) => css` - &:before { - content: ' '; - width: 14px; - mask-size: contain; - mask-repeat: no-repeat; - mask-position: center; - mask-image: url(${img}); - background: ${Colors.accentPrimary()}; - display: inline-block; - } -`; - -const InputDiv = styled.div` - ${SelectionAutoCompleteInputCSS} +const WrapperDiv = styled.div` .attribute-owner { ${iconStyle(Icons.owner.src)} } `; - -const GlobalHintStyles = createGlobalStyle` - .CodeMirror-hints { - background: ${Colors.popoverBackground()}; - border: none; - border-radius: 4px; - padding: 8px 4px; - .CodeMirror-hint { - border-radius: 4px; - font-size: 14px; - padding: 6px 8px 6px 12px; - color: ${Colors.textDefault()}; - &.CodeMirror-hint-active { - background-color: ${Colors.backgroundBlue()}; - color: ${Colors.textDefault()}; - } - } - } -`; - -function showHint(instance: Editor, hint: HintFunction) { - requestAnimationFrame(() => { - instance.showHint({ - hint, - completeSingle: false, - moveOnOverlap: true, - updateOnCursorActivity: true, - }); - }); -} diff --git a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoComplete.ts b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoComplete.ts index 8603de6447fe8..8973cd0835cdd 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoComplete.ts +++ b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoComplete.ts @@ -38,7 +38,7 @@ type TextCallback = (value: string) => string; const DEFAULT_TEXT_CALLBACK = (value: string) => value; // set to true for useful debug output. -const DEBUG = true; +const DEBUG = false; export class SelectionAutoCompleteVisitor extends AbstractParseTreeVisitor @@ -507,11 +507,15 @@ export class SelectionAutoCompleteVisitor } } -export function createSelectionHint, N extends keyof T>( - _nameBase: N, - attributesMap: T, - functions: string[], -): CodeMirror.HintFunction { +export function createSelectionHint, N extends keyof T>({ + nameBase: _nameBase, + attributesMap, + functions, +}: { + nameBase: N; + attributesMap: T; + functions: string[]; +}): CodeMirror.HintFunction { const nameBase = _nameBase as string; return function (cm: CodeMirror.Editor, _options: CodeMirror.ShowHintOptions): any { diff --git a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx new file mode 100644 index 0000000000000..feea0e8e15d71 --- /dev/null +++ b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx @@ -0,0 +1,210 @@ +import {Colors, Icon} from '@dagster-io/ui-components'; +import CodeMirror, {Editor, HintFunction} from 'codemirror'; +import {Linter} from 'codemirror/addon/lint/lint'; +import {useLayoutEffect, useMemo, useRef} from 'react'; +import styled, {createGlobalStyle, css} from 'styled-components'; + +import { + SelectionAutoCompleteInputCSS, + applyStaticSyntaxHighlighting, +} from './SelectionAutoCompleteHighlighter'; +import {useUpdatingRef} from '../hooks/useUpdatingRef'; +import {createSelectionHint} from '../selection/SelectionAutoComplete'; + +import 'codemirror/addon/edit/closebrackets'; +import 'codemirror/lib/codemirror.css'; +import 'codemirror/addon/hint/show-hint'; +import 'codemirror/addon/hint/show-hint.css'; +import 'codemirror/addon/lint/lint.css'; +import 'codemirror/addon/display/placeholder'; + +type SelectionAutoCompleteInputProps, N extends keyof T> = { + nameBase: N; + attributesMap: T; + placeholder: string; + functions: string[]; + linter: Linter; + value: string; + onChange: (value: string) => void; +}; + +export const SelectionAutoCompleteInput = , N extends keyof T>({ + value, + nameBase, + placeholder, + onChange, + functions, + linter, + attributesMap, +}: SelectionAutoCompleteInputProps) => { + const editorRef = useRef(null); + const cmInstance = useRef(null); + + const currentValueRef = useUpdatingRef(value); + const currentPendingValueRef = useRef(value); + const setValueTimeoutRef = useRef>(null); + + const hintRef = useUpdatingRef( + useMemo(() => { + return createSelectionHint({nameBase, attributesMap, functions}); + }, [nameBase, attributesMap, functions]), + ); + + useLayoutEffect(() => { + if (editorRef.current && !cmInstance.current) { + cmInstance.current = CodeMirror(editorRef.current, { + value, + mode: 'assetSelection', + lineNumbers: false, + lineWrapping: false, + scrollbarStyle: 'native', + autoCloseBrackets: true, + lint: { + getAnnotations: linter, + async: false, + }, + placeholder, + extraKeys: { + 'Ctrl-Space': 'autocomplete', + Tab: (cm: Editor) => { + cm.replaceSelection(' ', 'end'); + }, + }, + }); + + cmInstance.current.setSize('100%', 20); + + // Enforce single line by preventing newlines + cmInstance.current.on('beforeChange', (_instance: Editor, change) => { + if (change.text.some((line) => line.includes('\n'))) { + change.cancel(); + } + }); + + cmInstance.current.on('change', (instance: Editor, change) => { + const newValue = instance.getValue().replace(/\s+/g, ' '); + currentPendingValueRef.current = newValue; + if (setValueTimeoutRef.current) { + clearTimeout(setValueTimeoutRef.current); + } + setValueTimeoutRef.current = setTimeout(() => { + onChange(newValue); + }, 2000); + + if (change.origin === 'complete' && change.text[0]?.endsWith('()')) { + // Set cursor inside the right parenthesis + const cursor = instance.getCursor(); + instance.setCursor({...cursor, ch: cursor.ch - 1}); + } + }); + + cmInstance.current.on('inputRead', (instance: Editor) => { + showHint(instance, hintRef.current); + }); + + cmInstance.current.on('cursorActivity', (instance: Editor) => { + applyStaticSyntaxHighlighting(instance); + showHint(instance, hintRef.current); + }); + + cmInstance.current.on('blur', () => { + if (currentPendingValueRef.current !== currentValueRef.current) { + onChange(currentPendingValueRef.current); + } + }); + + requestAnimationFrame(() => { + if (!cmInstance.current) { + return; + } + + applyStaticSyntaxHighlighting(cmInstance.current); + }); + } + + const editor = editorRef.current; + return () => { + if (editor) { + editor.remove(); + } + }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + // Update CodeMirror when value prop changes + useLayoutEffect(() => { + const noNewLineValue = value.replace('\n', ' '); + if (cmInstance.current && cmInstance.current.getValue() !== noNewLineValue) { + const instance = cmInstance.current; + const cursor = instance.getCursor(); + instance.setValue(noNewLineValue); + instance.setCursor(cursor); + showHint(instance, hintRef.current); + } + }, [hintRef, value]); + + return ( + <> + + + +
+ + + ); +}; + +export const iconStyle = (img: string) => css` + &:before { + content: ' '; + width: 14px; + mask-size: contain; + mask-repeat: no-repeat; + mask-position: center; + mask-image: url(${img}); + background: ${Colors.accentPrimary()}; + display: inline-block; + } +`; + +const InputDiv = styled.div` + ${SelectionAutoCompleteInputCSS} +`; + +const GlobalHintStyles = createGlobalStyle` + .CodeMirror-hints { + background: ${Colors.popoverBackground()}; + border: none; + border-radius: 4px; + padding: 8px 4px; + .CodeMirror-hint { + border-radius: 4px; + font-size: 14px; + padding: 6px 8px 6px 12px; + color: ${Colors.textDefault()}; + &.CodeMirror-hint-active { + background-color: ${Colors.backgroundBlue()}; + color: ${Colors.textDefault()}; + } + } + } +`; + +function showHint(instance: Editor, hint: HintFunction) { + requestAnimationFrame(() => { + requestAnimationFrame(() => { + instance.showHint({ + hint, + completeSingle: false, + moveOnOverlap: true, + updateOnCursorActivity: true, + }); + }); + }); +} diff --git a/js_modules/dagster-ui/packages/ui-core/src/selection/__tests__/SelectionAutoComplete.test.ts b/js_modules/dagster-ui/packages/ui-core/src/selection/__tests__/SelectionAutoComplete.test.ts index 9e63b325f6d1a..88322779bd09a 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/selection/__tests__/SelectionAutoComplete.test.ts +++ b/js_modules/dagster-ui/packages/ui-core/src/selection/__tests__/SelectionAutoComplete.test.ts @@ -3,9 +3,9 @@ import {Hint, Hints, Position} from 'codemirror'; import {createSelectionHint} from '../SelectionAutoComplete'; describe('createAssetSelectionHint', () => { - const selectionHint = createSelectionHint( - 'key', - { + const selectionHint = createSelectionHint({ + nameBase: 'key', + attributesMap: { key: ['asset1', 'asset2', 'asset3'], tag: ['tag1', 'tag2', 'tag3'], owner: ['marco@dagsterlabs.com', 'team:frontend'], @@ -13,8 +13,8 @@ describe('createAssetSelectionHint', () => { kind: ['kind1', 'kind2'], code_location: ['repo1@location1', 'repo2@location2'], }, - ['sinks', 'roots'], - ); + functions: ['sinks', 'roots'], + }); type HintsModified = Omit & { list: Array; @@ -818,4 +818,65 @@ describe('createAssetSelectionHint', () => { to: 60, }); }); + + it('handles complex ands/ors', () => { + expect(testAutocomplete('key:"value"* or tag:"value"+ and owner:"owner" and |')).toEqual({ + from: 51, + list: [ + { + displayText: 'key_substring:', + text: 'key_substring:', + }, + { + displayText: 'key:', + text: 'key:', + }, + { + displayText: 'tag:', + text: 'tag:', + }, + { + displayText: 'owner:', + text: 'owner:', + }, + { + displayText: 'group:', + text: 'group:', + }, + { + displayText: 'kind:', + text: 'kind:', + }, + { + displayText: 'code_location:', + text: 'code_location:', + }, + { + displayText: 'sinks()', + text: 'sinks()', + }, + { + displayText: 'roots()', + text: 'roots()', + }, + { + displayText: 'not', + text: 'not ', + }, + { + displayText: '*', + text: '*', + }, + { + displayText: '+', + text: '+', + }, + { + displayText: '(', + text: '()', + }, + ], + to: 51, + }); + }); }); From 8b45cbb23677756aeb00f9d31e447a2e612cea02 Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Thu, 12 Dec 2024 11:14:50 -0500 Subject: [PATCH 02/11] refactor --- .../src/app/DefaultFeatureFlags.oss.tsx | 3 ++ .../input/AssetSelectionInput.oss.tsx | 11 ++++- .../input/AssetSelectionLinter.ts | 34 -------------- .../AssetSelectionSyntaxErrorListener.tsx | 26 ----------- .../selection/SelectionAutoCompleteInput.tsx | 10 ++-- .../src/selection/createSelectionLinter.ts | 46 +++++++++++++++++++ 6 files changed, 64 insertions(+), 66 deletions(-) delete mode 100644 js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionLinter.ts delete mode 100644 js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionSyntaxErrorListener.tsx create mode 100644 js_modules/dagster-ui/packages/ui-core/src/selection/createSelectionLinter.ts diff --git a/js_modules/dagster-ui/packages/ui-core/src/app/DefaultFeatureFlags.oss.tsx b/js_modules/dagster-ui/packages/ui-core/src/app/DefaultFeatureFlags.oss.tsx index 63a0632523530..4dab268bccfc8 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/app/DefaultFeatureFlags.oss.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/app/DefaultFeatureFlags.oss.tsx @@ -8,6 +8,9 @@ export const DEFAULT_FEATURE_FLAG_VALUES: Partial> [FeatureFlag.flagAssetSelectionSyntax]: new URLSearchParams(global?.location?.search ?? '').has( 'new-asset-selection-syntax', ), + [FeatureFlag.flagRunSelectionSyntax]: new URLSearchParams(global?.location?.search ?? '').has( + 'new-run-selection-syntax', + ), // Flags for tests [FeatureFlag.__TestFlagDefaultTrue]: true, diff --git a/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionInput.oss.tsx b/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionInput.oss.tsx index 24e10e52622a1..ef87451179f9b 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionInput.oss.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionInput.oss.tsx @@ -2,12 +2,14 @@ import {Icons} from '@dagster-io/ui-components'; import {useMemo} from 'react'; import styled from 'styled-components'; -import {lintAssetSelection} from './AssetSelectionLinter'; import {assertUnreachable} from '../../app/Util'; import {AssetGraphQueryItem} from '../../asset-graph/useAssetGraphData'; import {SelectionAutoCompleteInput, iconStyle} from '../../selection/SelectionAutoCompleteInput'; +import {createSelectionLinter} from '../../selection/createSelectionLinter'; import {placeholderTextForItems} from '../../ui/GraphQueryInput'; import {buildRepoPathForHuman} from '../../workspace/buildRepoAddress'; +import {AssetSelectionLexer} from '../generated/AssetSelectionLexer'; +import {AssetSelectionParser} from '../generated/AssetSelectionParser'; import 'codemirror/addon/edit/closebrackets'; import 'codemirror/lib/codemirror.css'; @@ -84,6 +86,11 @@ export const AssetSelectionInput = ({value, onChange, assets}: AssetSelectionInp code_location: codeLocations, }; }, [assets]); + + const linter = useMemo( + () => createSelectionLinter({Lexer: AssetSelectionLexer, Parser: AssetSelectionParser}), + [], + ); return ( diff --git a/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionLinter.ts b/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionLinter.ts deleted file mode 100644 index f1f1eb059c23e..0000000000000 --- a/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionLinter.ts +++ /dev/null @@ -1,34 +0,0 @@ -import {CharStreams, CommonTokenStream} from 'antlr4ts'; -import CodeMirror from 'codemirror'; - -import {AssetSelectionSyntaxErrorListener} from './AssetSelectionSyntaxErrorListener'; -import {AssetSelectionLexer} from '../generated/AssetSelectionLexer'; -import {AssetSelectionParser} from '../generated/AssetSelectionParser'; - -export const lintAssetSelection = (text: string) => { - const errorListener = new AssetSelectionSyntaxErrorListener(); - - const inputStream = CharStreams.fromString(text); - const lexer = new AssetSelectionLexer(inputStream); - - lexer.removeErrorListeners(); - lexer.addErrorListener(errorListener); - - const tokens = new CommonTokenStream(lexer); - const parser = new AssetSelectionParser(tokens); - - parser.removeErrorListeners(); // Remove default console error listener - parser.addErrorListener(errorListener); - - parser.start(); - - // Map syntax errors to CodeMirror's lint format - const lintErrors = errorListener.errors.map((error) => ({ - message: error.message.replace(', ', ''), - severity: 'error', - from: CodeMirror.Pos(error.line, error.column), - to: CodeMirror.Pos(error.line, text.length), - })); - - return lintErrors; -}; diff --git a/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionSyntaxErrorListener.tsx b/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionSyntaxErrorListener.tsx deleted file mode 100644 index 89d87f2dbf09e..0000000000000 --- a/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionSyntaxErrorListener.tsx +++ /dev/null @@ -1,26 +0,0 @@ -import {ANTLRErrorListener, RecognitionException, Recognizer} from 'antlr4ts'; - -interface SyntaxError { - message: string; - line: number; - column: number; -} - -export class AssetSelectionSyntaxErrorListener implements ANTLRErrorListener { - public errors: SyntaxError[] = []; - - syntaxError( - _recognizer: Recognizer, - _offendingSymbol: T | undefined, - line: number, - charPositionInLine: number, - msg: string, - _e: RecognitionException | undefined, - ): void { - this.errors.push({ - message: msg, - line: line - 1, // CodeMirror lines are 0-based - column: charPositionInLine, - }); - } -} diff --git a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx index feea0e8e15d71..e84b930cf248d 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx @@ -122,10 +122,12 @@ export const SelectionAutoCompleteInput = , N }); } - const editor = editorRef.current; return () => { - if (editor) { - editor.remove(); + const cm = cmInstance.current; + if (cm) { + // Clean up the instance... + cm.closeHint(); + cm.getWrapperElement()?.parentNode?.removeChild(cm.getWrapperElement()); } }; // eslint-disable-next-line react-hooks/exhaustive-deps @@ -173,7 +175,7 @@ export const iconStyle = (img: string) => css` } `; -const InputDiv = styled.div` +export const InputDiv = styled.div` ${SelectionAutoCompleteInputCSS} `; diff --git a/js_modules/dagster-ui/packages/ui-core/src/selection/createSelectionLinter.ts b/js_modules/dagster-ui/packages/ui-core/src/selection/createSelectionLinter.ts new file mode 100644 index 0000000000000..ac10d4c98f504 --- /dev/null +++ b/js_modules/dagster-ui/packages/ui-core/src/selection/createSelectionLinter.ts @@ -0,0 +1,46 @@ +import {CharStreams, CommonTokenStream, Lexer, Parser, ParserRuleContext} from 'antlr4ts'; +import CodeMirror from 'codemirror'; +import {Linter} from 'codemirror/addon/lint/lint'; + +import {CustomErrorListener} from './CustomErrorListener'; + +type LexerConstructor = new (...args: any[]) => Lexer; +type ParserConstructor = new (...args: any[]) => Parser & { + start: () => ParserRuleContext; +}; + +export function createSelectionLinter({ + Lexer: LexerKlass, + Parser: ParserKlass, +}: { + Lexer: LexerConstructor; + Parser: ParserConstructor; +}): Linter { + return (text: string) => { + const errorListener = new CustomErrorListener(); + + const inputStream = CharStreams.fromString(text); + const lexer = new LexerKlass(inputStream); + + lexer.removeErrorListeners(); + lexer.addErrorListener(errorListener); + + const tokens = new CommonTokenStream(lexer); + const parser = new ParserKlass(tokens); + + parser.removeErrorListeners(); // Remove default console error listener + parser.addErrorListener(errorListener); + + parser.start(); + + // Map syntax errors to CodeMirror's lint format + const lintErrors = errorListener.getErrors().map((error) => ({ + message: error.message.replace(', ', ''), + severity: 'error', + from: CodeMirror.Pos(error.line, error.column), + to: CodeMirror.Pos(error.line, text.length), + })); + + return lintErrors; + }; +} From 24ef26c42e7f016a7bec813b07047a2f8df36fb7 Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Thu, 19 Dec 2024 13:47:16 -0500 Subject: [PATCH 03/11] weak map memoize --- .../src/util/__tests__/weakMapMemoize.test.ts | 261 ++++++++++++++++++ .../ui-core/src/util/weakMapMemoize.ts | 66 +++++ 2 files changed, 327 insertions(+) create mode 100644 js_modules/dagster-ui/packages/ui-core/src/util/__tests__/weakMapMemoize.test.ts create mode 100644 js_modules/dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts diff --git a/js_modules/dagster-ui/packages/ui-core/src/util/__tests__/weakMapMemoize.test.ts b/js_modules/dagster-ui/packages/ui-core/src/util/__tests__/weakMapMemoize.test.ts new file mode 100644 index 0000000000000..140d846d2e58e --- /dev/null +++ b/js_modules/dagster-ui/packages/ui-core/src/util/__tests__/weakMapMemoize.test.ts @@ -0,0 +1,261 @@ +import {weakMapMemoize} from '../weakMapMemoize'; + +type AnyFunction = (...args: any[]) => any; + +describe('weakMapMemoize', () => { + // Test 1: Function with primitive arguments + it('should memoize correctly with primitive arguments and avoid redundant calls', () => { + const spy = jest.fn((a: number, b: number) => a + b); + const memoizedAdd = weakMapMemoize(spy); + + const result1 = memoizedAdd(1, 2); + const result2 = memoizedAdd(1, 2); + const result3 = memoizedAdd(2, 3); + + expect(result1).toBe(3); + expect(result2).toBe(3); + expect(result3).toBe(5); + expect(spy).toHaveBeenCalledTimes(2); // Only two unique calls + }); + + // Test 2: Function with object arguments + it('should memoize correctly based on object references', () => { + const spy = jest.fn((obj: {x: number}, y: number) => obj.x + y); + const memoizedFn = weakMapMemoize(spy); + + const obj1 = {x: 10}; + const obj2 = {x: 20}; + + const result1 = memoizedFn(obj1, 5); + const result2 = memoizedFn(obj1, 5); + const result3 = memoizedFn(obj2, 5); + const result4 = memoizedFn(obj1, 6); + + expect(result1).toBe(15); + expect(result2).toBe(15); + expect(result3).toBe(25); + expect(result4).toBe(16); + expect(spy).toHaveBeenCalledTimes(3); // Three unique calls + }); + + // Test 3: Function with mixed arguments + it('should memoize correctly with mixed primitive and object arguments', () => { + const spy = jest.fn((a: number, obj: {y: number}) => a + obj.y); + const memoizedFn = weakMapMemoize(spy); + + const obj1 = {y: 100}; + const obj2 = {y: 200}; + + const result1 = memoizedFn(1, obj1); + const result2 = memoizedFn(1, obj1); + const result3 = memoizedFn(2, obj1); + const result4 = memoizedFn(1, obj2); + + expect(result1).toBe(101); + expect(result2).toBe(101); + expect(result3).toBe(102); + expect(result4).toBe(201); + expect(spy).toHaveBeenCalledTimes(3); // Three unique calls + }); + + // Test 4: Function with no arguments + it('should memoize the result when function has no arguments', () => { + const spy = jest.fn(() => Math.random()); + const memoizedFn = weakMapMemoize(spy); + + const result1 = memoizedFn(); + const result2 = memoizedFn(); + const result3 = memoizedFn(); + + expect(result1).toBe(result2); + expect(result2).toBe(result3); + expect(spy).toHaveBeenCalledTimes(1); // Only one unique call + }); + + // Test 5: Function with null and undefined arguments + it('should handle null and undefined arguments correctly', () => { + const spy = jest.fn((a: any, b: any) => { + if (a === null && b === undefined) { + return 'null-undefined'; + } + return 'other'; + }); + const memoizedFn = weakMapMemoize(spy); + + const result1 = memoizedFn(null, undefined); + const result2 = memoizedFn(null, undefined); + const result3 = memoizedFn(undefined, null); + const result4 = memoizedFn(null, undefined); + + expect(result1).toBe('null-undefined'); + expect(result2).toBe('null-undefined'); + expect(result3).toBe('other'); + expect(result4).toBe('null-undefined'); + expect(spy).toHaveBeenCalledTimes(2); // Two unique calls + }); + + // Test 6: Function with function arguments + it('should memoize based on function references', () => { + const spy = jest.fn((fn: AnyFunction, value: number) => fn(value)); + const memoizedFn = weakMapMemoize(spy); + + const func1 = (x: number) => x * 2; + const func2 = (x: number) => x * 3; + + const result1 = memoizedFn(func1, 5); + const result2 = memoizedFn(func1, 5); + const result3 = memoizedFn(func2, 5); + const result4 = memoizedFn(func1, 6); + + expect(result1).toBe(10); + expect(result2).toBe(10); + expect(result3).toBe(15); + expect(result4).toBe(12); + expect(spy).toHaveBeenCalledTimes(3); // Three unique calls + }); + + // Test 7: Function with multiple mixed arguments + it('should memoize correctly with multiple mixed argument types', () => { + const spy = jest.fn((a: number, b: string, c: {key: string}) => `${a}-${b}-${c.key}`); + const memoizedFn = weakMapMemoize(spy); + + const obj1 = {key: 'value1'}; + const obj2 = {key: 'value2'}; + + const result1 = memoizedFn(1, 'test', obj1); + const result2 = memoizedFn(1, 'test', obj1); + const result3 = memoizedFn(1, 'test', obj2); + const result4 = memoizedFn(2, 'test', obj1); + + expect(result1).toBe('1-test-value1'); + expect(result2).toBe('1-test-value1'); + expect(result3).toBe('1-test-value2'); + expect(result4).toBe('2-test-value1'); + expect(spy).toHaveBeenCalledTimes(3); // Three unique calls + }); + + // Test 8: Function with array arguments + it('should memoize based on array references', () => { + const spy = jest.fn((arr: number[]) => arr.reduce((sum, val) => sum + val, 0)); + const memoizedFn = weakMapMemoize(spy); + + const array1 = [1, 2, 3]; + const array2 = [4, 5, 6]; + + const result1 = memoizedFn(array1); + const result2 = memoizedFn(array1); + const result3 = memoizedFn(array2); + const result4 = memoizedFn([1, 2, 3]); // Different reference + + expect(result1).toBe(6); + expect(result2).toBe(6); + expect(result3).toBe(15); + expect(result4).toBe(6); + expect(spy).toHaveBeenCalledTimes(3); // Three unique calls + }); + + // Test 9: Function with symbols as arguments + it('should memoize based on symbol references', () => { + const sym1 = Symbol('sym1'); + const sym2 = Symbol('sym2'); + + const spy = jest.fn((a: symbol, b: number) => a.toString() + b); + const memoizedFn = weakMapMemoize(spy); + + const result1 = memoizedFn(sym1, 10); + const result2 = memoizedFn(sym1, 10); + const result3 = memoizedFn(sym2, 10); + const result4 = memoizedFn(sym1, 20); + + expect(result1).toBe(`${sym1.toString()}10`); + expect(result2).toBe(`${sym1.toString()}10`); + expect(result3).toBe(`${sym2.toString()}10`); + expect(result4).toBe(`${sym1.toString()}20`); + expect(spy).toHaveBeenCalledTimes(3); // Three unique calls + }); + + // Test 10: Function with a large number of arguments + it('should memoize correctly with a large number of arguments', () => { + const spy = jest.fn((...args: number[]) => args.reduce((sum, val) => sum + val, 0)); + const memoizedFn = weakMapMemoize(spy); + + const args1 = [1, 2, 3, 4, 5]; + const args2 = [1, 2, 3, 4, 5]; + const args3 = [5, 4, 3, 2, 1]; + const args4 = [1, 2, 3, 4, 6]; + + const result1 = memoizedFn(...args1); + const result2 = memoizedFn(...args2); + const result3 = memoizedFn(...args3); + const result4 = memoizedFn(...args4); + + expect(result1).toBe(15); + expect(result2).toBe(15); + expect(result3).toBe(15); + expect(result4).toBe(16); + expect(spy).toHaveBeenCalledTimes(3); // Three unique calls + }); + + // Test 11: Function with alternating object and primitive arguments + it('should memoize correctly with alternating object and primitive arguments', () => { + const spy = jest.fn( + ( + obj1: {a: number}, + prim1: string, + obj2: {b: number}, + prim2: boolean, + obj3: {c: number}, + prim3: number, + ) => obj1.a + prim1.length + obj2.b + (prim2 ? 1 : 0) + obj3.c + prim3, + ); + const memoizedFn = weakMapMemoize(spy); + + const object1 = {a: 5}; + const object2 = {b: 10}; + const object3 = {c: 15}; + + // First unique call + const result1 = memoizedFn(object1, 'test', object2, true, object3, 20); + + // Duplicate of the first call + const result2 = memoizedFn(object1, 'test', object2, true, object3, 20); + + // Different primitive in second argument + const result3 = memoizedFn(object1, 'testing', object2, true, object3, 20); + + // Different object in third argument + const object4 = {b: 20}; + const result4 = memoizedFn(object1, 'test', object4, true, object3, 20); + + // Different primitive in fourth argument + const result5 = memoizedFn(object1, 'test', object2, false, object3, 20); + + // Different object in fifth argument + const object5 = {c: 25}; + const result6 = memoizedFn(object1, 'test', object2, true, object5, 20); + + // Different primitive in sixth argument + const result7 = memoizedFn(object1, 'test', object2, true, object3, 30); + + // Different objects and primitives + const result8 = memoizedFn(object1, 'testing', object2, false, object5, 30); + + // Duplicate of the first call again + const result9 = memoizedFn(object1, 'test', object2, true, object3, 20); + + expect(result1).toBe(5 + 4 + 10 + 1 + 15 + 20); // 5 + 4 + 10 + 1 + 15 + 20 = 55 + expect(result2).toBe(55); // Cached + expect(result3).toBe(5 + 7 + 10 + 1 + 15 + 20); // 5 + 7 + 10 + 1 + 15 + 20 = 58 + expect(result4).toBe(5 + 4 + 20 + 1 + 15 + 20); // 5 + 4 + 20 + 1 + 15 + 20 = 65 + expect(result5).toBe(5 + 4 + 10 + 0 + 15 + 20); // 5 + 4 + 10 + 0 + 15 + 20 = 54 + expect(result6).toBe(5 + 4 + 10 + 1 + 25 + 20); // 5 + 4 + 10 + 1 + 25 + 20 = 65 + expect(result7).toBe(5 + 4 + 10 + 1 + 15 + 30); // 5 + 4 + 10 + 1 + 15 + 30 = 65 + expect(result8).toBe(5 + 7 + 10 + 0 + 25 + 30); // 5 + 7 + 25 + 0 + 15 + 30 = 97 + expect(result9).toBe(55); // Cached + + // spy should be called for each unique combination + // Unique calls: result1, result3, result4, result5, result6, result7, result8 + // Total unique calls: 7 + expect(spy).toHaveBeenCalledTimes(7); + }); +}); diff --git a/js_modules/dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts b/js_modules/dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts new file mode 100644 index 0000000000000..2e898f4bd1e58 --- /dev/null +++ b/js_modules/dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts @@ -0,0 +1,66 @@ +type AnyFunction = (...args: any[]) => any; + +function isObject(value: any): value is object { + return value !== null && (typeof value === 'object' || typeof value === 'function'); +} + +/** + * Interface representing a cache node that holds both a Map for primitive keys + * and a WeakMap for object keys. + */ +interface CacheNode { + map: Map; + weakMap: WeakMap; + result?: any; +} + +/** + * Memoizes a function using nested Maps and WeakMaps based on the arguments. + * Handles both primitive and object arguments. + * @param fn - The function to memoize. + * @returns A memoized version of the function. + */ +export function weakMapMemoize(fn: T): T { + // Initialize the root cache node + const cacheRoot: CacheNode = { + map: new Map(), + weakMap: new WeakMap(), + }; + + return function memoizedFunction(this: any, ...args: any[]) { + let currentCache = cacheRoot; + + for (let i = 0; i < args.length; i++) { + const arg = args[i]; + + if (isObject(arg)) { + // Use WeakMap for object keys + if (!currentCache.weakMap.has(arg)) { + currentCache.weakMap.set(arg, { + map: new Map(), + weakMap: new WeakMap(), + }); + } + currentCache = currentCache.weakMap.get(arg)!; + } else { + // Use Map for primitive keys + if (!currentCache.map.has(arg)) { + currentCache.map.set(arg, { + map: new Map(), + weakMap: new WeakMap(), + }); + } + currentCache = currentCache.map.get(arg)!; + } + } + + // After traversing all arguments, check if the result is cached + if ('result' in currentCache) { + return currentCache.result; + } + // Compute the result and cache it + const result = fn.apply(this, args); + currentCache.result = result; + return result; + } as T; +} From 5d97f0b930d6d62f994ca8517b4434c3c6e861a0 Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Thu, 19 Dec 2024 15:27:59 -0500 Subject: [PATCH 04/11] add max entries options --- .../src/util/__tests__/weakMapMemoize.test.ts | 89 +++++++++++------ .../ui-core/src/util/weakMapMemoize.ts | 99 ++++++++++++++++--- 2 files changed, 142 insertions(+), 46 deletions(-) diff --git a/js_modules/dagster-ui/packages/ui-core/src/util/__tests__/weakMapMemoize.test.ts b/js_modules/dagster-ui/packages/ui-core/src/util/__tests__/weakMapMemoize.test.ts index 140d846d2e58e..12009237b32bc 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/util/__tests__/weakMapMemoize.test.ts +++ b/js_modules/dagster-ui/packages/ui-core/src/util/__tests__/weakMapMemoize.test.ts @@ -6,7 +6,7 @@ describe('weakMapMemoize', () => { // Test 1: Function with primitive arguments it('should memoize correctly with primitive arguments and avoid redundant calls', () => { const spy = jest.fn((a: number, b: number) => a + b); - const memoizedAdd = weakMapMemoize(spy); + const memoizedAdd = weakMapMemoize(spy, {maxEntries: 3}); const result1 = memoizedAdd(1, 2); const result2 = memoizedAdd(1, 2); @@ -21,7 +21,7 @@ describe('weakMapMemoize', () => { // Test 2: Function with object arguments it('should memoize correctly based on object references', () => { const spy = jest.fn((obj: {x: number}, y: number) => obj.x + y); - const memoizedFn = weakMapMemoize(spy); + const memoizedFn = weakMapMemoize(spy, {maxEntries: 2}); const obj1 = {x: 10}; const obj2 = {x: 20}; @@ -41,7 +41,7 @@ describe('weakMapMemoize', () => { // Test 3: Function with mixed arguments it('should memoize correctly with mixed primitive and object arguments', () => { const spy = jest.fn((a: number, obj: {y: number}) => a + obj.y); - const memoizedFn = weakMapMemoize(spy); + const memoizedFn = weakMapMemoize(spy, {maxEntries: 3}); const obj1 = {y: 100}; const obj2 = {y: 200}; @@ -61,7 +61,7 @@ describe('weakMapMemoize', () => { // Test 4: Function with no arguments it('should memoize the result when function has no arguments', () => { const spy = jest.fn(() => Math.random()); - const memoizedFn = weakMapMemoize(spy); + const memoizedFn = weakMapMemoize(spy, {maxEntries: 1}); const result1 = memoizedFn(); const result2 = memoizedFn(); @@ -80,7 +80,7 @@ describe('weakMapMemoize', () => { } return 'other'; }); - const memoizedFn = weakMapMemoize(spy); + const memoizedFn = weakMapMemoize(spy, {maxEntries: 2}); const result1 = memoizedFn(null, undefined); const result2 = memoizedFn(null, undefined); @@ -97,7 +97,7 @@ describe('weakMapMemoize', () => { // Test 6: Function with function arguments it('should memoize based on function references', () => { const spy = jest.fn((fn: AnyFunction, value: number) => fn(value)); - const memoizedFn = weakMapMemoize(spy); + const memoizedFn = weakMapMemoize(spy, {maxEntries: 3}); const func1 = (x: number) => x * 2; const func2 = (x: number) => x * 3; @@ -117,7 +117,7 @@ describe('weakMapMemoize', () => { // Test 7: Function with multiple mixed arguments it('should memoize correctly with multiple mixed argument types', () => { const spy = jest.fn((a: number, b: string, c: {key: string}) => `${a}-${b}-${c.key}`); - const memoizedFn = weakMapMemoize(spy); + const memoizedFn = weakMapMemoize(spy, {maxEntries: 4}); const obj1 = {key: 'value1'}; const obj2 = {key: 'value2'}; @@ -137,7 +137,7 @@ describe('weakMapMemoize', () => { // Test 8: Function with array arguments it('should memoize based on array references', () => { const spy = jest.fn((arr: number[]) => arr.reduce((sum, val) => sum + val, 0)); - const memoizedFn = weakMapMemoize(spy); + const memoizedFn = weakMapMemoize(spy, {maxEntries: 3}); const array1 = [1, 2, 3]; const array2 = [4, 5, 6]; @@ -160,7 +160,7 @@ describe('weakMapMemoize', () => { const sym2 = Symbol('sym2'); const spy = jest.fn((a: symbol, b: number) => a.toString() + b); - const memoizedFn = weakMapMemoize(spy); + const memoizedFn = weakMapMemoize(spy, {maxEntries: 4}); const result1 = memoizedFn(sym1, 10); const result2 = memoizedFn(sym1, 10); @@ -177,23 +177,29 @@ describe('weakMapMemoize', () => { // Test 10: Function with a large number of arguments it('should memoize correctly with a large number of arguments', () => { const spy = jest.fn((...args: number[]) => args.reduce((sum, val) => sum + val, 0)); - const memoizedFn = weakMapMemoize(spy); + const memoizedFn = weakMapMemoize(spy, {maxEntries: 5}); const args1 = [1, 2, 3, 4, 5]; const args2 = [1, 2, 3, 4, 5]; const args3 = [5, 4, 3, 2, 1]; const args4 = [1, 2, 3, 4, 6]; + const args5 = [6, 5, 4, 3, 2]; + const args6 = [1, 2, 3, 4, 7]; const result1 = memoizedFn(...args1); const result2 = memoizedFn(...args2); const result3 = memoizedFn(...args3); const result4 = memoizedFn(...args4); + const result5 = memoizedFn(...args5); + const result6 = memoizedFn(...args6); expect(result1).toBe(15); expect(result2).toBe(15); expect(result3).toBe(15); expect(result4).toBe(16); - expect(spy).toHaveBeenCalledTimes(3); // Three unique calls + expect(result5).toBe(20); + expect(result6).toBe(17); + expect(spy).toHaveBeenCalledTimes(5); // Five unique calls (args1, args3, args4, args5, args6) }); // Test 11: Function with alternating object and primitive arguments @@ -208,49 +214,48 @@ describe('weakMapMemoize', () => { prim3: number, ) => obj1.a + prim1.length + obj2.b + (prim2 ? 1 : 0) + obj3.c + prim3, ); - const memoizedFn = weakMapMemoize(spy); + const memoizedFn = weakMapMemoize(spy, {maxEntries: 7}); const object1 = {a: 5}; const object2 = {b: 10}; const object3 = {c: 15}; + const object4 = {b: 20}; // Corrected to have 'b' property + const object5 = {c: 25}; // Corrected to have 'c' property // First unique call - const result1 = memoizedFn(object1, 'test', object2, true, object3, 20); + const result1 = memoizedFn(object1, 'test', object2, true, object3, 20); // 5 +4 +10 +1 +15 +20 =55 // Duplicate of the first call - const result2 = memoizedFn(object1, 'test', object2, true, object3, 20); + const result2 = memoizedFn(object1, 'test', object2, true, object3, 20); // 55 // Different primitive in second argument - const result3 = memoizedFn(object1, 'testing', object2, true, object3, 20); + const result3 = memoizedFn(object1, 'testing', object2, true, object3, 20); //5 +7 +10 +1 +15 +20=58 // Different object in third argument - const object4 = {b: 20}; - const result4 = memoizedFn(object1, 'test', object4, true, object3, 20); + const result4 = memoizedFn(object1, 'test', object4, true, object3, 20); //5 +4 +20 +1 +15 +20=65 // Different primitive in fourth argument - const result5 = memoizedFn(object1, 'test', object2, false, object3, 20); + const result5 = memoizedFn(object1, 'test', object2, false, object3, 20); //5 +4 +10 +0 +15 +20=54 // Different object in fifth argument - const object5 = {c: 25}; - const result6 = memoizedFn(object1, 'test', object2, true, object5, 20); + const result6 = memoizedFn(object1, 'test', object2, true, object5, 20); //5 +4 +10 +1 +25 +20=65 // Different primitive in sixth argument - const result7 = memoizedFn(object1, 'test', object2, true, object3, 30); + const result7 = memoizedFn(object1, 'test', object2, true, object3, 30); //5 +4 +10 +1 +15 +30=65 // Different objects and primitives - const result8 = memoizedFn(object1, 'testing', object2, false, object5, 30); + const result8 = memoizedFn(object1, 'testing', object2, false, object3, 30); //5 +7 +10 +0 +15 +30=67 // Duplicate of the first call again const result9 = memoizedFn(object1, 'test', object2, true, object3, 20); - - expect(result1).toBe(5 + 4 + 10 + 1 + 15 + 20); // 5 + 4 + 10 + 1 + 15 + 20 = 55 + expect(result1).toBe(5 + 4 + 10 + 1 + 15 + 20); // 55 expect(result2).toBe(55); // Cached - expect(result3).toBe(5 + 7 + 10 + 1 + 15 + 20); // 5 + 7 + 10 + 1 + 15 + 20 = 58 - expect(result4).toBe(5 + 4 + 20 + 1 + 15 + 20); // 5 + 4 + 20 + 1 + 15 + 20 = 65 - expect(result5).toBe(5 + 4 + 10 + 0 + 15 + 20); // 5 + 4 + 10 + 0 + 15 + 20 = 54 - expect(result6).toBe(5 + 4 + 10 + 1 + 25 + 20); // 5 + 4 + 10 + 1 + 25 + 20 = 65 - expect(result7).toBe(5 + 4 + 10 + 1 + 15 + 30); // 5 + 4 + 10 + 1 + 15 + 30 = 65 - expect(result8).toBe(5 + 7 + 10 + 0 + 25 + 30); // 5 + 7 + 25 + 0 + 15 + 30 = 97 + expect(result3).toBe(5 + 7 + 10 + 1 + 15 + 20); // 58 + expect(result4).toBe(5 + 4 + 20 + 1 + 15 + 20); // 65 + expect(result5).toBe(5 + 4 + 10 + 0 + 15 + 20); // 54 + expect(result6).toBe(5 + 4 + 10 + 1 + 25 + 20); // 65 + expect(result7).toBe(5 + 4 + 10 + 1 + 15 + 30); // 65 + expect(result8).toBe(5 + 7 + 10 + 0 + 15 + 30); // 67 expect(result9).toBe(55); // Cached // spy should be called for each unique combination @@ -258,4 +263,28 @@ describe('weakMapMemoize', () => { // Total unique calls: 7 expect(spy).toHaveBeenCalledTimes(7); }); + + // Test 12: Exercising the maxEntries option + it('should evict least recently used entries when maxEntries is exceeded', () => { + const spy = jest.fn((a: number) => a * 2); + const memoizedFn = weakMapMemoize(spy, {maxEntries: 2}); + + const result1 = memoizedFn(1); // Cached + const result2 = memoizedFn(2); // Cached + const result3 = memoizedFn(3); // Evicts least recently used (1) + const result4 = memoizedFn(2); // Cached, updates recentness + const result5 = memoizedFn(4); // Evicts least recently used (3) + + expect(result1).toBe(2); + expect(result2).toBe(4); + expect(result3).toBe(6); + expect(result4).toBe(4); + expect(result5).toBe(8); + expect(spy).toHaveBeenCalledTimes(4); // Calls for 1,2,3,4 + + // Accessing 1 again should trigger a new call since it was evicted + const result6 = memoizedFn(1); + expect(result6).toBe(2); + expect(spy).toHaveBeenCalledTimes(5); // Call for 1 again + }); }); diff --git a/js_modules/dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts b/js_modules/dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts index 2e898f4bd1e58..862fc5e6a78ce 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts +++ b/js_modules/dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts @@ -1,54 +1,105 @@ +import LRU from 'lru-cache'; + type AnyFunction = (...args: any[]) => any; -function isObject(value: any): value is object { - return value !== null && (typeof value === 'object' || typeof value === 'function'); +interface WeakMapMemoizeOptions { + maxEntries?: number; // Optional limit for cached entries } -/** - * Interface representing a cache node that holds both a Map for primitive keys - * and a WeakMap for object keys. - */ interface CacheNode { map: Map; weakMap: WeakMap; result?: any; + lruKey?: any; // Reference to the key in the LRU cache +} + +/** + * Determines if a value is a non-null object or function. + * @param value - The value to check. + * @returns True if the value is a non-null object or function, false otherwise. + */ +function isObject(value: any): value is object { + return value !== null && (typeof value === 'object' || typeof value === 'function'); } /** * Memoizes a function using nested Maps and WeakMaps based on the arguments. - * Handles both primitive and object arguments. + * Optionally limits the number of cached entries using an LRU cache. + * Handles both primitive and object arguments efficiently. * @param fn - The function to memoize. + * @param options - Optional settings for memoization. * @returns A memoized version of the function. */ -export function weakMapMemoize(fn: T): T { +export function weakMapMemoize(fn: T, options?: WeakMapMemoizeOptions): T { + const {maxEntries} = options || {}; + // Initialize the root cache node const cacheRoot: CacheNode = { map: new Map(), weakMap: new WeakMap(), }; + // Initialize LRU Cache if maxEntries is specified + let lruCache: LRU.Cache | null = null; + + if (maxEntries) { + lruCache = new LRU({ + max: maxEntries, + dispose: (key, value) => { + // When an entry is evicted from the LRU cache, + // traverse the cache tree and remove the cached result + const keyPath = key as any[]; + let currentCache = cacheRoot; + + for (let i = 0; i < keyPath.length; i++) { + const arg = keyPath[i]; + const isArgObject = isObject(arg); + + if (isArgObject) { + currentCache = currentCache.weakMap.get(arg); + } else { + currentCache = currentCache.map.get(arg); + } + + if (!currentCache) { + // The cache node has already been removed + return; + } + } + + // Remove the cached result + delete currentCache.result; + delete currentCache.lruKey; + }, + }); + } + return function memoizedFunction(this: any, ...args: any[]) { let currentCache = cacheRoot; + const path: any[] = []; for (let i = 0; i < args.length; i++) { const arg = args[i]; + path.push(arg); + const isArgObject = isObject(arg); - if (isObject(arg)) { - // Use WeakMap for object keys + // Determine the appropriate cache level + if (isArgObject) { if (!currentCache.weakMap.has(arg)) { - currentCache.weakMap.set(arg, { + const newCacheNode: CacheNode = { map: new Map(), weakMap: new WeakMap(), - }); + }; + currentCache.weakMap.set(arg, newCacheNode); } currentCache = currentCache.weakMap.get(arg)!; } else { - // Use Map for primitive keys if (!currentCache.map.has(arg)) { - currentCache.map.set(arg, { + const newCacheNode: CacheNode = { map: new Map(), weakMap: new WeakMap(), - }); + }; + currentCache.map.set(arg, newCacheNode); } currentCache = currentCache.map.get(arg)!; } @@ -56,11 +107,27 @@ export function weakMapMemoize(fn: T): T { // After traversing all arguments, check if the result is cached if ('result' in currentCache) { + // If using LRU Cache, update its usage + if (lruCache && currentCache.lruKey) { + lruCache.get(currentCache.lruKey); // This updates the recentness + } return currentCache.result; } - // Compute the result and cache it + + // Compute the result const result = fn.apply(this, args); + + // Cache the result currentCache.result = result; + + // If LRU cache is enabled, manage the cache entries + if (lruCache) { + const cacheEntryKey: any[] = path.slice(); // Clone the path to ensure uniqueness + currentCache.lruKey = cacheEntryKey; // Associate the cache node with the LRU key + + lruCache.set(cacheEntryKey, currentCache); + } + return result; } as T; } From d6fd380e0aadd4f2cbd0c5abcd8a1feef7a6a14e Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Thu, 19 Dec 2024 18:53:30 -0500 Subject: [PATCH 05/11] ts --- .../dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/js_modules/dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts b/js_modules/dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts index 862fc5e6a78ce..8699464a1479a 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts +++ b/js_modules/dagster-ui/packages/ui-core/src/util/weakMapMemoize.ts @@ -40,16 +40,16 @@ export function weakMapMemoize(fn: T, options?: WeakMapMe }; // Initialize LRU Cache if maxEntries is specified - let lruCache: LRU.Cache | null = null; + let lruCache: LRU | null = null; if (maxEntries) { lruCache = new LRU({ max: maxEntries, - dispose: (key, value) => { + dispose: (key, _value) => { // When an entry is evicted from the LRU cache, // traverse the cache tree and remove the cached result const keyPath = key as any[]; - let currentCache = cacheRoot; + let currentCache: CacheNode | undefined = cacheRoot; for (let i = 0; i < keyPath.length; i++) { const arg = keyPath[i]; From 7262f1e5a1c978cfcc56275ba67d687fe0d93369 Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Fri, 20 Dec 2024 10:24:32 -0500 Subject: [PATCH 06/11] gant chart selection --- .../asset-selection/AntlrAssetSelection.ts | 27 ++--- .../packages/ui-core/src/gantt/GanttChart.tsx | 27 +++-- .../src/gantt/GanttChartSelectionInput.tsx | 62 +++++++++++ .../src/run-selection/AntlrRunSelection.ts | 29 ++--- .../run-selection/AntlrRunSelectionVisitor.ts | 8 +- .../packages/ui-core/src/runs/Run.tsx | 101 +++++++++++++----- .../ui-core/src/runs/RunActionButtons.tsx | 10 +- .../selection/SelectionAutoCompleteInput.tsx | 14 +-- 8 files changed, 207 insertions(+), 71 deletions(-) create mode 100644 js_modules/dagster-ui/packages/ui-core/src/gantt/GanttChartSelectionInput.tsx diff --git a/js_modules/dagster-ui/packages/ui-core/src/asset-selection/AntlrAssetSelection.ts b/js_modules/dagster-ui/packages/ui-core/src/asset-selection/AntlrAssetSelection.ts index d39434b2d0201..9a32b9ba87f68 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/asset-selection/AntlrAssetSelection.ts +++ b/js_modules/dagster-ui/packages/ui-core/src/asset-selection/AntlrAssetSelection.ts @@ -9,6 +9,7 @@ import {FeatureFlag} from 'shared/app/FeatureFlags.oss'; import {AntlrAssetSelectionVisitor} from './AntlrAssetSelectionVisitor'; import {AssetGraphQueryItem} from '../asset-graph/useAssetGraphData'; +import {weakMapMemoize} from '../util/weakMapMemoize'; import {AssetSelectionLexer} from './generated/AssetSelectionLexer'; import {AssetSelectionParser} from './generated/AssetSelectionParser'; import {featureEnabled} from '../app/Flags'; @@ -65,17 +66,17 @@ export const parseAssetSelectionQuery = ( } }; -export const filterAssetSelectionByQuery = ( - all_assets: AssetGraphQueryItem[], - query: string, -): AssetSelectionQueryResult => { - if (featureEnabled(FeatureFlag.flagAssetSelectionSyntax)) { - const result = parseAssetSelectionQuery(all_assets, query); - if (result instanceof Error) { - // fall back to old behavior - return filterByQuery(all_assets, query); +export const filterAssetSelectionByQuery = weakMapMemoize( + (all_assets: AssetGraphQueryItem[], query: string): AssetSelectionQueryResult => { + if (featureEnabled(FeatureFlag.flagAssetSelectionSyntax)) { + const result = parseAssetSelectionQuery(all_assets, query); + if (result instanceof Error) { + // fall back to old behavior + return filterByQuery(all_assets, query); + } + return result; } - return result; - } - return filterByQuery(all_assets, query); -}; + return filterByQuery(all_assets, query); + }, + {maxEntries: 20}, +); diff --git a/js_modules/dagster-ui/packages/ui-core/src/gantt/GanttChart.tsx b/js_modules/dagster-ui/packages/ui-core/src/gantt/GanttChart.tsx index 39b4fcdc0f4e8..179e15ac0d2ef 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/gantt/GanttChart.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/gantt/GanttChart.tsx @@ -15,6 +15,7 @@ import isEqual from 'lodash/isEqual'; import * as React from 'react'; import {useMemo} from 'react'; import {Link} from 'react-router-dom'; +import {FeatureFlag} from 'shared/app/FeatureFlags.oss'; import styled from 'styled-components'; import { @@ -48,6 +49,7 @@ import { interestingQueriesFor, } from './GanttChartLayout'; import {GanttChartModeControl} from './GanttChartModeControl'; +import {GanttChartSelectionInput} from './GanttChartSelectionInput'; import {GanttChartTimescale} from './GanttChartTimescale'; import {GanttStatusPanel} from './GanttStatusPanel'; import {OptionsContainer, OptionsSpacer} from './VizComponents'; @@ -55,6 +57,7 @@ import {ZoomSlider} from './ZoomSlider'; import {RunGraphQueryItem} from './toGraphQueryItems'; import {useGanttChartMode} from './useGanttChartMode'; import {AppContext} from '../app/AppContext'; +import {featureEnabled} from '../app/Flags'; import {GraphQueryItem} from '../app/GraphQueryImpl'; import {withMiddleTruncation} from '../app/Util'; import {WebSocketContext} from '../app/WebSocketProvider'; @@ -411,14 +414,22 @@ const GanttChartInner = React.memo((props: GanttChartInnerProps) => { ) : null} - 0 ? 'has-step' : ''} - /> + {featureEnabled(FeatureFlag.flagRunSelectionSyntax) ? ( + + ) : ( + 0 ? 'has-step' : ''} + /> + )} void; +}) => { + const attributesMap = useMemo(() => { + const statuses = new Set(); + const names = new Set(); + + items.forEach((item) => { + if (item.metadata?.state) { + statuses.add(item.metadata.state); + } else { + statuses.add(NO_STATE); + } + names.add(item.name); + }); + return {name: Array.from(names), status: Array.from(statuses)}; + }, [items]); + + return ( + + + + ); +}; + +const getLinter = weakMapMemoize(() => + createSelectionLinter({Lexer: RunSelectionLexer, Parser: RunSelectionParser}), +); + +const FUNCTIONS = ['sinks', 'roots']; + +const Wrapper = styled.div` + ${InputDiv} { + width: 24vw; + } +`; diff --git a/js_modules/dagster-ui/packages/ui-core/src/run-selection/AntlrRunSelection.ts b/js_modules/dagster-ui/packages/ui-core/src/run-selection/AntlrRunSelection.ts index ae19f75aa464b..d93abfb242675 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/run-selection/AntlrRunSelection.ts +++ b/js_modules/dagster-ui/packages/ui-core/src/run-selection/AntlrRunSelection.ts @@ -8,8 +8,9 @@ import {RunSelectionLexer} from './generated/RunSelectionLexer'; import {RunSelectionParser} from './generated/RunSelectionParser'; import {featureEnabled} from '../app/Flags'; import {filterByQuery} from '../app/GraphQueryImpl'; +import {weakMapMemoize} from '../util/weakMapMemoize'; -type RunSelectionQueryResult = { +export type RunSelectionQueryResult = { all: RunGraphQueryItem[]; focus: RunGraphQueryItem[]; }; @@ -44,17 +45,17 @@ export const parseRunSelectionQuery = ( } }; -export const filterRunSelectionByQuery = ( - all_runs: RunGraphQueryItem[], - query: string, -): RunSelectionQueryResult => { - if (featureEnabled(FeatureFlag.flagRunSelectionSyntax)) { - const result = parseRunSelectionQuery(all_runs, query); - if (result instanceof Error) { - // fall back to old behavior - return filterByQuery(all_runs, query); +export const filterRunSelectionByQuery = weakMapMemoize( + (all_runs: RunGraphQueryItem[], query: string): RunSelectionQueryResult => { + if (featureEnabled(FeatureFlag.flagRunSelectionSyntax)) { + const result = parseRunSelectionQuery(all_runs, query); + if (result instanceof Error) { + // fall back to old behavior + return filterByQuery(all_runs, query); + } + return result; } - return result; - } - return filterByQuery(all_runs, query); -}; + return filterByQuery(all_runs, query); + }, + {maxEntries: 20}, +); diff --git a/js_modules/dagster-ui/packages/ui-core/src/run-selection/AntlrRunSelectionVisitor.ts b/js_modules/dagster-ui/packages/ui-core/src/run-selection/AntlrRunSelectionVisitor.ts index c1f457693062a..97983129733f9 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/run-selection/AntlrRunSelectionVisitor.ts +++ b/js_modules/dagster-ui/packages/ui-core/src/run-selection/AntlrRunSelectionVisitor.ts @@ -160,6 +160,12 @@ export class AntlrRunSelectionVisitor visitStatusAttributeExpr(ctx: StatusAttributeExprContext) { const state: string = getValue(ctx.value()).toLowerCase(); - return new Set([...this.all_runs].filter((i) => i.metadata?.state === state)); + return new Set( + [...this.all_runs].filter( + (i) => i.metadata?.state === state || (state === NO_STATE && !i.metadata?.state), + ), + ); } } + +export const NO_STATE = 'none'; diff --git a/js_modules/dagster-ui/packages/ui-core/src/runs/Run.tsx b/js_modules/dagster-ui/packages/ui-core/src/runs/Run.tsx index 0d227c3fa1bde..fde9c0f26bd65 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/runs/Run.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/runs/Run.tsx @@ -10,7 +10,8 @@ import { Tooltip, } from '@dagster-io/ui-components'; import * as React from 'react'; -import {memo} from 'react'; +import {memo, useLayoutEffect, useMemo, useRef, useState} from 'react'; +import {FeatureFlag} from 'shared/app/FeatureFlags.oss'; import styled from 'styled-components'; import {CapturedOrExternalLogPanel} from './CapturedLogPanel'; @@ -27,7 +28,7 @@ import { } from './useComputeLogFileKeyForSelection'; import {useQueryPersistedLogFilter} from './useQueryPersistedLogFilter'; import {showCustomAlert} from '../app/CustomAlertProvider'; -import {filterByQuery} from '../app/GraphQueryImpl'; +import {featureEnabled} from '../app/Flags'; import {PythonErrorInfo} from '../app/PythonErrorInfo'; import {isHiddenAssetGroupJob} from '../asset-graph/Utils'; import {GanttChart, GanttChartLoadingState, GanttChartMode, QueuedState} from '../gantt/GanttChart'; @@ -37,6 +38,7 @@ import {useDocumentTitle} from '../hooks/useDocumentTitle'; import {useFavicon} from '../hooks/useFavicon'; import {useQueryPersistedState} from '../hooks/useQueryPersistedState'; import {CompletionType, useTraceDependency} from '../performance/TraceContext'; +import {filterRunSelectionByQuery} from '../run-selection/AntlrRunSelection'; interface RunProps { runId: string; @@ -127,7 +129,7 @@ export const Run = memo((props: RunProps) => { }); const OnLogsLoaded = ({dependency}: {dependency: ReturnType}) => { - React.useLayoutEffect(() => { + useLayoutEffect(() => { dependency.completeDependency(CompletionType.SUCCESS); }, [dependency]); return null; @@ -179,6 +181,8 @@ const RunWithData = ({ onSetLogsFilter, onSetSelectionQuery, }: RunWithDataProps) => { + const newRunSelectionSyntax = featureEnabled(FeatureFlag.flagRunSelectionSyntax); + const [queryLogType, setQueryLogType] = useQueryPersistedState({ queryKey: 'logType', defaults: {logType: LogType.structured}, @@ -186,19 +190,27 @@ const RunWithData = ({ const logType = logTypeFromQuery(queryLogType); const setLogType = (lt: LogType) => setQueryLogType(LogType[lt]); - const [computeLogUrl, setComputeLogUrl] = React.useState(null); + const [computeLogUrl, setComputeLogUrl] = useState(null); const stepKeysJSON = JSON.stringify(Object.keys(metadata.steps).sort()); - const stepKeys = React.useMemo(() => JSON.parse(stepKeysJSON), [stepKeysJSON]); + const stepKeys = useMemo(() => JSON.parse(stepKeysJSON), [stepKeysJSON]); const runtimeGraph = run?.executionPlan && toGraphQueryItems(run?.executionPlan, metadata.steps); - const selectionStepKeys = React.useMemo(() => { + const selectionStepKeys = useMemo(() => { return runtimeGraph && selectionQuery && selectionQuery !== '*' - ? filterByQuery(runtimeGraph, selectionQuery).all.map((n) => n.name) + ? filterRunSelectionByQuery(runtimeGraph, selectionQuery).all.map((n) => n.name) : []; }, [runtimeGraph, selectionQuery]); + const selection = useMemo( + () => ({ + query: selectionQuery, + keys: selectionStepKeys, + }), + [selectionStepKeys, selectionQuery], + ); + const {logCaptureInfo, computeLogFileKey, setComputeLogFileKey} = useComputeLogFileKeyForSelection({ stepKeys, @@ -207,19 +219,26 @@ const RunWithData = ({ defaultToFirstStep: false, }); - const logsFilterStepKeys = runtimeGraph - ? logsFilter.logQuery - .filter((v) => v.token && v.token === 'query') - .reduce((accum, v) => { - accum.push(...filterByQuery(runtimeGraph, v.value).all.map((n) => n.name)); - return accum; - }, [] as string[]) - : []; + const logsFilterStepKeys = useMemo( + () => + runtimeGraph + ? logsFilter.logQuery + .filter((v) => v.token && v.token === 'query') + .reduce((accum, v) => { + accum.push( + ...filterRunSelectionByQuery(runtimeGraph, v.value).all.map((n) => n.name), + ); + return accum; + }, [] as string[]) + : [], + [logsFilter.logQuery, runtimeGraph], + ); const onClickStep = (stepKey: string, evt: React.MouseEvent) => { const index = selectionStepKeys.indexOf(stepKey); - let newSelected: string[]; + let newSelected: string[] = []; const filterForExactStep = `"${stepKey}"`; + let nextSelectionQuery = selectionQuery; if (evt.shiftKey) { // shift-click to multi select steps, preserving quotations if present newSelected = [ @@ -228,18 +247,34 @@ const RunWithData = ({ if (index !== -1) { // deselect the step if already selected - newSelected.splice(index, 1); + if (newRunSelectionSyntax) { + nextSelectionQuery = removeStepFromSelection(nextSelectionQuery, stepKey); + } else { + newSelected.splice(index, 1); + } } else { // select the step otherwise - newSelected.push(filterForExactStep); + if (newRunSelectionSyntax) { + nextSelectionQuery = addStepToSelection(nextSelectionQuery, stepKey); + } else { + newSelected.push(filterForExactStep); + } } } else { + // deselect the step if already selected if (selectionStepKeys.length === 1 && index !== -1) { - // deselect the step if already selected - newSelected = []; + if (newRunSelectionSyntax) { + nextSelectionQuery = ''; + } else { + newSelected = []; + } } else { // select the step otherwise - newSelected = [filterForExactStep]; + if (newRunSelectionSyntax) { + nextSelectionQuery = `name:"${stepKey}"`; + } else { + newSelected = [filterForExactStep]; + } // When only one step is selected, set the compute log key as well. const matchingLogKey = matchingComputeLogKeyFromStepKey(metadata.logCaptureSteps, stepKey); @@ -249,13 +284,17 @@ const RunWithData = ({ } } - onSetSelectionQuery(newSelected.join(', ') || '*'); + if (newRunSelectionSyntax) { + onSetSelectionQuery(nextSelectionQuery); + } else { + onSetSelectionQuery(newSelected.join(', ') || '*'); + } }; - const [expandedPanel, setExpandedPanel] = React.useState(null); - const containerRef = React.useRef(null); + const [expandedPanel, setExpandedPanel] = useState(null); + const containerRef = useRef(null); - React.useEffect(() => { + useLayoutEffect(() => { if (containerRef.current) { const size = containerRef.current.getSize(); if (size === 100) { @@ -310,14 +349,14 @@ const RunWithData = ({ run={run} graph={runtimeGraph} metadata={metadata} - selection={{query: selectionQuery, keys: selectionStepKeys}} + selection={selection} /> } runId={runId} graph={runtimeGraph} metadata={metadata} - selection={{query: selectionQuery, keys: selectionStepKeys}} + selection={selection} onClickStep={onClickStep} onSetSelection={onSetSelectionQuery} focusedTime={logsFilter.focusedTime} @@ -409,3 +448,11 @@ const NoStepSelectionState = ({type}: {type: LogType}) => { ); }; + +function removeStepFromSelection(selectionQuery: string, stepKey: string) { + return `(${selectionQuery}) and not name:"${stepKey}"`; +} + +function addStepToSelection(selectionQuery: string, stepKey: string) { + return `(${selectionQuery}) or name:"${stepKey}"`; +} diff --git a/js_modules/dagster-ui/packages/ui-core/src/runs/RunActionButtons.tsx b/js_modules/dagster-ui/packages/ui-core/src/runs/RunActionButtons.tsx index 4143f0cc979d3..5d520b7d0bc5a 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/runs/RunActionButtons.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/runs/RunActionButtons.tsx @@ -1,5 +1,6 @@ import {Box, Button, Group, Icon} from '@dagster-io/ui-components'; import {useCallback, useState} from 'react'; +import {FeatureFlag} from 'shared/app/FeatureFlags.oss'; import {IRunMetadataDict, IStepState} from './RunMetadataProvider'; import {doneStatuses, failedStatuses} from './RunStatuses'; @@ -11,10 +12,12 @@ import {RunFragment, RunPageFragment} from './types/RunFragments.types'; import {useJobAvailabilityErrorForRun} from './useJobAvailabilityErrorForRun'; import {useJobReexecution} from './useJobReExecution'; import {showSharedToaster} from '../app/DomUtils'; +import {featureEnabled} from '../app/Flags'; import {GraphQueryItem, filterByQuery} from '../app/GraphQueryImpl'; import {DEFAULT_DISABLED_REASON} from '../app/Permissions'; import {ReexecutionStrategy} from '../graphql/types'; import {LaunchButtonConfiguration, LaunchButtonDropdown} from '../launchpad/LaunchButton'; +import {filterRunSelectionByQuery} from '../run-selection/AntlrRunSelection'; import {useRepositoryForRunWithParentSnapshot} from '../workspace/useRepositoryForRun'; interface RunActionButtonsProps { @@ -185,8 +188,11 @@ export const RunActionButtons = (props: RunActionButtonsProps) => { console.warn('Run execution plan must be present to launch from-selected execution'); return Promise.resolve(); } - const selectionAndDownstreamQuery = selection.keys.map((k) => `${k}*`).join(','); - const selectionKeys = filterByQuery(graph, selectionAndDownstreamQuery).all.map( + const selectionAndDownstreamQuery = featureEnabled(FeatureFlag.flagRunSelectionSyntax) + ? selection.keys.map((k) => `name:"${k}"*`).join(' or ') + : selection.keys.map((k) => `${k}*`).join(','); + + const selectionKeys = filterRunSelectionByQuery(graph, selectionAndDownstreamQuery).all.map( (node) => node.name, ); diff --git a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx index e84b930cf248d..ecacc7bbc834a 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx @@ -201,12 +201,14 @@ const GlobalHintStyles = createGlobalStyle` function showHint(instance: Editor, hint: HintFunction) { requestAnimationFrame(() => { requestAnimationFrame(() => { - instance.showHint({ - hint, - completeSingle: false, - moveOnOverlap: true, - updateOnCursorActivity: true, - }); + if (instance.getWrapperElement().contains(document.activeElement)) { + instance.showHint({ + hint, + completeSingle: false, + moveOnOverlap: true, + updateOnCursorActivity: true, + }); + } }); }); } From 3eee5f6c417e69a2b96fce0d31d2e330e11c8468 Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Fri, 20 Dec 2024 10:55:22 -0500 Subject: [PATCH 07/11] 1 more ting --- .../dagster-ui/packages/ui-core/src/runs/Run.tsx | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/js_modules/dagster-ui/packages/ui-core/src/runs/Run.tsx b/js_modules/dagster-ui/packages/ui-core/src/runs/Run.tsx index fde9c0f26bd65..4fd391633cf5c 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/runs/Run.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/runs/Run.tsx @@ -105,7 +105,7 @@ export const Run = memo((props: RunProps) => { {(logs) => ( <> - + {(metadata) => ( { ); }); -const OnLogsLoaded = ({dependency}: {dependency: ReturnType}) => { +const OnLogsLoaded = ({ + dependency, + logs, +}: { + dependency: ReturnType; + logs: LogsProviderLogs; +}) => { useLayoutEffect(() => { - dependency.completeDependency(CompletionType.SUCCESS); - }, [dependency]); + if (!logs.loading) { + dependency.completeDependency(CompletionType.SUCCESS); + } + }, [dependency, logs]); return null; }; From 62e94582373da9310449fab9b09566b3b45f5679 Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Fri, 20 Dec 2024 11:06:36 -0500 Subject: [PATCH 08/11] 1 more ting --- .../packages/ui-core/src/gantt/GanttChartSelectionInput.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/js_modules/dagster-ui/packages/ui-core/src/gantt/GanttChartSelectionInput.tsx b/js_modules/dagster-ui/packages/ui-core/src/gantt/GanttChartSelectionInput.tsx index 13deb20c9419b..e6c373fb2163f 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/gantt/GanttChartSelectionInput.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/gantt/GanttChartSelectionInput.tsx @@ -2,11 +2,10 @@ import {useMemo} from 'react'; import styled from 'styled-components'; import {RunGraphQueryItem} from './toGraphQueryItems'; -import {SelectionAutoCompleteInput} from '../../../../../../../internal/dagster-cloud/js_modules/app-cloud/dagster/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput'; import {NO_STATE} from '../run-selection/AntlrRunSelectionVisitor'; import {RunSelectionLexer} from '../run-selection/generated/RunSelectionLexer'; import {RunSelectionParser} from '../run-selection/generated/RunSelectionParser'; -import {InputDiv} from '../selection/SelectionAutoCompleteInput'; +import {InputDiv, SelectionAutoCompleteInput} from '../selection/SelectionAutoCompleteInput'; import {createSelectionLinter} from '../selection/createSelectionLinter'; import {weakMapMemoize} from '../util/weakMapMemoize'; From 798f21f5fea13185804de20a04ee4948209ecb99 Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Fri, 20 Dec 2024 14:49:24 -0500 Subject: [PATCH 09/11] 1 more ting --- .../ui-core/src/selection/SelectionAutoCompleteInput.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx index ecacc7bbc834a..e021299a40ee4 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInput.tsx @@ -179,8 +179,10 @@ export const InputDiv = styled.div` ${SelectionAutoCompleteInputCSS} `; +// Z-index: 21 to beat out Dialog's z-index: 20 const GlobalHintStyles = createGlobalStyle` .CodeMirror-hints { + z-index: 21; background: ${Colors.popoverBackground()}; border: none; border-radius: 4px; From a5f00ed1cd664db139851ec1d79d260fdacaeacd Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Fri, 20 Dec 2024 23:30:30 -0500 Subject: [PATCH 10/11] . --- .../ui-core/src/assets/VirtualizedSimpleAssetKeyList.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js_modules/dagster-ui/packages/ui-core/src/assets/VirtualizedSimpleAssetKeyList.tsx b/js_modules/dagster-ui/packages/ui-core/src/assets/VirtualizedSimpleAssetKeyList.tsx index 7404111fc5014..0e5e7bf441615 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/assets/VirtualizedSimpleAssetKeyList.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/assets/VirtualizedSimpleAssetKeyList.tsx @@ -13,7 +13,7 @@ export const VirtualizedSimpleAssetKeyList = ({ style, }: { assetKeys: AssetKeyInput[]; - style: CSSProperties; + style?: CSSProperties; }) => { const parentRef = useRef(null); const rowVirtualizer = useVirtualizer({ From 655eaf483a27e8f87a4be524a08cd298b767ea6e Mon Sep 17 00:00:00 2001 From: Marco Salazar Date: Mon, 23 Dec 2024 10:51:19 -0500 Subject: [PATCH 11/11] include workspace context change --- .../WorkspaceContext/WorkspaceContext.tsx | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/js_modules/dagster-ui/packages/ui-core/src/workspace/WorkspaceContext/WorkspaceContext.tsx b/js_modules/dagster-ui/packages/ui-core/src/workspace/WorkspaceContext/WorkspaceContext.tsx index 4870011c261eb..cd60b1e776c55 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/workspace/WorkspaceContext/WorkspaceContext.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/workspace/WorkspaceContext/WorkspaceContext.tsx @@ -62,9 +62,18 @@ interface WorkspaceState { setHidden: SetVisibleOrHiddenFn; } -export const WorkspaceContext = React.createContext( - new Error('WorkspaceContext should never be uninitialized') as any, -); +export const WorkspaceContext = React.createContext({ + allRepos: [], + visibleRepos: [], + data: {}, + refetch: () => Promise.reject(), + toggleVisible: () => {}, + loading: false, + locationEntries: [], + locationStatuses: {}, + setVisible: () => {}, + setHidden: () => {}, +}); interface BaseLocationParams { localCacheIdPrefix?: string;