feat(editors): Add @omniview/editors package with production-grade components#20
Merged
Conversation
New package at packages/editors/ with CodeEditor, DiffViewer, Terminal, MarkdownPreview, CommandPalette, and ObjectInspector components. - Theme integration via MutationObserver on data-ov-theme attribute - Monaco themes built from --ov-syntax-* and --ov-color-editor-* tokens - xterm themes built from --ov-color-terminal-* and --ov-color-ansi-* tokens - Heavy deps (Monaco, xterm) lazy-loaded via dynamic import + Suspense - 48 tests across all 6 components, all passing - Storybook stories with autodocs for every component
…ed JSON Terminal rewrite: - Debounced resize (10ms) via ResizeObserver + window resize listener - WebGL → Canvas → DOM renderer cascade with context loss recovery - WebLinksAddon for clickable URLs, Mac-specific options by default - Binary data support (Uint8Array), configurable scrollback buffer - Signal/error/close callback props for exec plugin integration - Additional imperative methods: getDimensions, reset, scrollToBottom - Stable callback refs to avoid terminal re-initialization - Proper disposal order: debounce → observer → listeners → addons → terminal - 26 comprehensive tests covering resize, binary writes, cleanup, etc. ObjectInspector: circular reference detection prevents infinite recursion, clipboard error handling, toYaml circular ref guard. CodeEditor: memoized JSON pretty-printing to avoid re-parsing on every render.
…stics API - Downgrade monaco-editor to 0.52.2 for monaco-yaml@5.4.1 compatibility - Add EditorSchemaRegistry with runtime YAML/JSON schema registration - Add onDiagnostics/onCursorChange callbacks and getDebugState() handle - Fix model disposal (detach before dispose) to prevent Canceled errors - Fix test mocks for monaco.languages.typescript (0.52 API shape) - Add resolve alias for monaco-editor ESM entry in Vite config - Suppress marked.js sourcemap warning in Storybook via Vite plugin - Document monaco-yaml compatibility issues and fork considerations
…DE API surface
- Upgrade all xterm dependencies to latest (xterm 6.0, addons 0.11-0.19)
- Remove @xterm/addon-canvas (dropped in v6), simplify renderer to WebGL → DOM
- Fix CSS rendering bug: import xterm.css, set monospace font-family on container
- Add onReady callback for session attach timing, autoFocus prop
- Add fontWeight/fontWeightBold, drawBoldTextInBrightColors, onWriteParsed props
- Expand handle: blur(), search (findNext/findPrevious/clearSearch), selection,
scroll (scrollToLine/scrollUp/scrollDown), paste, getBufferContent via SerializeAddon
- Load Unicode11Addon for proper emoji/CJK width, await document.fonts.ready
- Export TerminalErrorInfo type matching Go SDK StreamError for ERROR signal
- Wire all xterm events (onBell, onTitleChange, onKey, onSelectionChange, etc.)
- Add renderer prop ('auto'|'webgl'|'dom'), disableStdin, customKeyEventHandler
- Dynamic option updates: fontSize, cursorBlink, cursorStyle, scrollback, lineHeight, letterSpacing
- New stories: Search, ReadOnly, RendererComparison, Serialization, CustomKeyHandler
- 79 terminal tests covering lifecycle, resize debounce, cleanup ordering, race conditions
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as React App
participant CodeEditor as CodeEditor
participant MonacoEnv as setupMonacoWorkers / MonacoEnvironment
participant Workers as Language Workers (yaml/json/ts/...)
participant SchemaReg as EditorSchemaRegistry
App->>CodeEditor: mount(filename, content)
CodeEditor->>MonacoEnv: ensure workers (setupMonacoWorkers)
MonacoEnv->>Workers: instantiate language workers
MonacoEnv->>SchemaReg: inject YAML handle
CodeEditor->>SchemaReg: getSchemasForUri(modelUri)
SchemaReg-->>CodeEditor: return matching schemas
CodeEditor->>Workers: applyYamlSchemas / applyJsonSchemas
App->>CodeEditor: update content
CodeEditor->>MonacoEnv: read markers/diagnostics
CodeEditor-->>App: onDiagnostics(EditorDiagnostic[])
sequenceDiagram
participant App as React App
participant Terminal as Terminal
participant Xterm as xterm.js
participant Addons as Addons (Fit/Search/Serialize/...)
participant Theme as useEditorTheme / token getters
App->>Terminal: render(props)
Terminal->>Theme: get current theme (useEditorTheme)
Terminal->>Xterm: lazy-load & instantiate Terminal(options)
Terminal->>Addons: load & attach addons
App->>Terminal: call handle.write(data)
Xterm-->>Terminal: onData -> Terminal forwards to App callback
App->>Terminal: unmount
Terminal->>Addons: dispose()
Terminal->>Xterm: dispose()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 36
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/theme/styles.css`:
- Around line 424-511: The issue is missing theme overrides for
editor/terminal/syntax tokens (e.g., --ov-color-editor-bg, --ov-color-editor-fg,
--ov-color-terminal-bg, --ov-color-terminal-fg, and --ov-syntax-*) which are
only set in :root (dark); add corresponding overrides under
:root[data-ov-theme='light'], [data-ov-theme='high-contrast-dark'], and
[data-ov-theme='high-contrast-light'] selectors, setting light-friendly
background/foreground and adjusted syntax colors and higher-contrast values for
high-contrast themes so Monaco editor and terminal use the correct palettes
(update the same tokens listed in the diff such as --ov-color-editor-bg,
--ov-color-editor-selection-bg, --ov-color-terminal-border, --ov-syntax-comment,
--ov-syntax-string, --ov-syntax-keyword, etc.).
In `@packages/editors/docs/MONACO_YAML_COMPAT.md`:
- Around line 29-31: The fenced code blocks in MONACO_YAML_COMPAT.md are missing
language specifiers causing static-analysis warnings; update the three affected
code fences (the block showing "TypeError: Cannot use 'in' operator to search
for 'then' in undefined" and the blocks around lines ~56-59 and ~100-102) by
adding a language token such as text or plaintext to the opening ``` (e.g.,
change ``` to ```text) for each of those three fenced code blocks so they are
properly annotated.
In `@packages/editors/src/components/code-editor/CodeEditor.stories.tsx`:
- Around line 505-508: The nested ternary in CodeEditor.stories.tsx using
loaded[activeGvr], activeGvr, and loadedCount is hard to read; extract the logic
into a small helper (e.g., getActiveFileSchemaLabel or computeActiveSchemaLabel)
that returns the correct string for the UI and then call that helper in the JSX
so the div shows "Loaded: …" followed by the helper's result; ensure the helper
covers the three cases: loaded[activeGvr] truthy -> " — active file has schema",
loaded[activeGvr] === undefined && loadedCount > 0 -> " — active file has NO
schema loaded", otherwise "".
- Around line 429-456: The buttons in the CodeEditor stories are missing an
explicit type, causing them to default to type="submit" inside forms; update the
JSX button elements (the mapped GVR buttons that call loadSchema and
setActiveGvr/setValue, the "Unload All" button, and the GVR selector buttons) to
include type="button" so they do not submit forms or reload the page while
keeping their existing onClick, disabled, and styling logic (references:
loadSchema, setActiveGvr, setValue, loaded, loading, sampleYamlByGvr).
In `@packages/editors/src/components/code-editor/CodeEditor.test.tsx`:
- Around line 205-208: Update the test in CodeEditor.test.tsx to also assert the
Monaco model is disposed on unmount: spy or mock the call to createModel (the
mocked createModel helper) so you can capture its return value (the mockModel),
render the <CodeEditor value="" /> and then call unmount(), and finally add an
expectation that mockModel.dispose was called in addition to
expect(mockEditorInstance.dispose).toHaveBeenCalled(); ensure you reference the
mocked createModel return (mockModel) and its dispose method in the assertion.
In `@packages/editors/src/components/code-editor/CodeEditor.tsx`:
- Around line 315-393: The mount useEffect in CodeEditor.tsx intentionally uses
an empty dependency array (and an eslint-disable comment) so props like
diagnostics, readOnly, lineNumbers, minimap, wordWrap, quickSuggestions,
tabSize, filename, and displayValue are captured only at mount and updated
elsewhere; add a short explanatory comment above this useEffect (referencing the
useEffect that creates the Monaco editor, editorRef, and the return cleanup)
stating this "mount once" design and pointing maintainers to the other effects
that handle prop updates, so the eslint-disable is clearly intentional and not
accidental.
- Around line 407-413: The current call uses editor.getModel()! and then
getFullModelRange(), which can throw if the model is null; update the CodeEditor
logic around editor.executeEdits to first obtain the model via const model =
editor.getModel() and bail out or skip the executeEdits call if model is null
(or set the model safely) so you never call model.getFullModelRange() on a null;
ensure you still use the same displayValue and forceMoveMarkers when model
exists and reference editor.executeEdits, editor.getModel, and getFullModelRange
in the guarded/conditional flow.
- Around line 454-462: The swap-model logic may create a new model with stale
content because currentValue falls back to displayValue; change the source to
read the editor buffer directly by using editor.getValue() as the primary
content before detaching the model (fall back to currentModel?.getValue?.() or
displayValue if editor.getValue() isn't available), then pass that content into
getOrCreateModel(filename, resolvedLanguage, ...) and set the new model; update
the code around currentModel/currentValue/getOrCreateModel/editor.setModel to
use editor.getValue() so in-progress edits aren't lost.
In `@packages/editors/src/components/command-palette/CommandPalette.module.css`:
- Line 8: Replace the hardcoded background rgba(0, 0, 0, 0.4) in the
CommandPalette overlay with a CSS variable (e.g., --overlay-bg) and reference
that variable where the background is set (keeping a fallback to the current
rgba value). Add the new variable definition to the module-level or global theme
variables (e.g., :root or the existing theme container used by CommandPalette)
so light/dark themes can override it (and ensure components that consume this
module use the updated variable name).
In `@packages/editors/src/components/command-palette/CommandPalette.stories.tsx`:
- Line 51: The buttons in CommandPalette.stories.tsx are missing explicit type
attributes (they default to type="submit")—update every <button> in this story
to include type="button" to avoid accidental form submissions; specifically add
type="button" to the button with onClick={() => setOpen(true)} and the other
button elements referenced in this file (the ones around the handlers/labels at
the locations noted in the review) so each button element explicitly reads
type="button".
In `@packages/editors/src/components/command-palette/CommandPalette.test.tsx`:
- Around line 10-12: Update the vi.mock factory for '@omniview/base-ui' to be
async and import React via await vi.importActual('react') instead of require;
remove the unused parameter named _ik from the mocked function signature, and
make the element that currently has role="option" focusable by adding
tabIndex="0" (or replace it with a proper focusable option container) so the
mock complies with a11y and lint rules—target the vi.mock('@omniview/base-ui',
...) factory, the unused _ik parameter, and the mocked element that sets
role="option".
In `@packages/editors/src/components/command-palette/CommandPalette.tsx`:
- Around line 46-59: Add keyboard handlers to make the CommandPalette
accessible: on the overlay div (the element with className={styles.Overlay} and
data-testid="command-palette-overlay") add an onKeyDown handler that listens for
Escape and calls onClose so keyboard users can dismiss the palette; on the inner
dialog div (the element with ref={ref}, className={styles.Root} and
data-testid="command-palette") add an onKeyDown that stops propagation of
keyboard events (mirroring the existing onClick stopPropagation) and ensure the
element is not inadvertently focusable (e.g., set tabIndex={-1}) so it remains a
static dialog container rather than an interactive control.
In `@packages/editors/src/components/diff-viewer/DiffViewer.test.tsx`:
- Around line 18-42: The test currently stubs createDiffEditor/createModel but
returns ad-hoc model objects, so it only asserts
mockDiffEditorInstance.dispose() and misses verifying that the two models
created by createModel are disposed; update the test to capture the two model
instances returned by createModel (reference the createModel mock and its
returned objects), make mockDiffEditorInstance.getModel() return those same
created models (so getModel() yields { original, modified }), then on unmount
assert that both original.dispose() and modified.dispose() were called in
addition to mockDiffEditorInstance.dispose(); ensure you modify the mocks used
in the DiffViewer.test.tsx (createDiffEditor, createModel,
mockDiffEditorInstance, getModel, dispose) so the test fails on regressions in
model cleanup.
- Line 38: The createModel mock currently declares unused parameters (_value,
_lang) causing linter warnings; change the mock implementation referenced by
createModel to be a zero-argument vi.fn() (e.g., vi.fn(() => ({ ... }))) so it
no longer defines unused params but still returns the same object and continues
to record call arguments for assertions like toHaveBeenCalledWith on
createModel.
In `@packages/editors/src/components/diff-viewer/DiffViewer.tsx`:
- Around line 104-109: The early return in the useEffect blocks on "!language"
prevents resetting a model to plaintext; remove the "!language" check from the
guard in the useEffect so the effect runs even when language is undefined, and
keep the existing calls to monaco.editor.setModelLanguage(models.original,
language) and monaco.editor.setModelLanguage(models.modified, language) so
undefined can propagate and allow reverting typed models to plaintext;
references: useEffect, editorRef.current.getModel(),
models.original/models.modified, monaco.editor.setModelLanguage, isReady,
language.
In `@packages/editors/src/components/index.ts`:
- Around line 1-6: Replace the wildcard barrel exports with explicit named
exports so the package API is intentional: instead of "export * from
'./code-editor';", "export * from './diff-viewer';", "export * from
'./terminal';", "export * from './markdown-preview';", "export * from
'./command-palette';", and "export * from './object-inspector';" list and
re-export only the public symbols from each module (e.g., export { CodeEditor,
CodeEditorProps } from './code-editor'; export { DiffViewer, DiffViewerProps }
from './diff-viewer'; export { Terminal, TerminalProps } from './terminal';
export { MarkdownPreview } from './markdown-preview'; export { CommandPalette }
from './command-palette'; export { ObjectInspector } from './object-inspector');
update the lists to match the actual exported identifiers in each file and add
new symbols here explicitly when they become part of the public API.
In
`@packages/editors/src/components/markdown-preview/MarkdownPreview.stories.tsx`:
- Around line 69-74: The template literal in fetchPods is using triple-escaped
backticks (\\\`) which produces a backslash+backtick instead of a proper
`${namespace}` interpolation; update the call to client.get in fetchPods to use
a normal backtick-delimited template string like
`/api/v1/namespaces/${namespace}/pods` (remove the extra backslashes/escapes) so
the ${namespace} interpolation works and the returned PodListResponse is fetched
correctly.
In `@packages/editors/src/components/markdown-preview/MarkdownPreview.test.tsx`:
- Around line 109-112: Replace synchronous getByTestId assertions for the mocked
react-markdown with the async pattern: mark the affected tests (e.g., the
"passes content to the markdown renderer" test and any other tests asserting on
'react-markdown-mock') as async, use await
screen.findByTestId('react-markdown-mock') to obtain the element and then run
your expect(...).toHaveTextContent / other assertions; update all assertions
referencing 'react-markdown-mock' in MarkdownPreview.test.tsx accordingly so
tests await the async lookup instead of using getByTestId.
In `@packages/editors/src/components/markdown-preview/MarkdownPreview.tsx`:
- Around line 86-118: The generated itemId in the details renderer (variable
itemId derived from summaryText in the details: ({ children, node }: MdProps) =>
{ ... }) can collide for identical summaries; modify the id generation so
Accordion.Item receives a stable unique id by appending a short unique suffix
(e.g. a module-level incrementing counter or a UUID/random suffix) to the
slugified summaryText used for itemId, or use a React-stable id helper (like
useId in a wrapper) to ensure uniqueness across multiple <details> elements;
update the code that constructs itemId and passes it to <Accordion.Item
id={itemId}> accordingly.
In `@packages/editors/src/components/object-inspector/ObjectInspector.module.css`:
- Around line 105-121: The CSS rules for .Value[data-type='string'],
.Value[data-type='number'], .Value[data-type='boolean'], and
.Value[data-type='null']/undefined use hardcoded hex fallbacks that assume a
dark theme; replace those hardcoded fallbacks by either ensuring the
corresponding CSS custom properties (--ov-syntax-string, --ov-syntax-number,
--ov-syntax-keyword) are defined for both themes or change the fallback to a
theme-agnostic value (e.g., currentColor or inherit) so the values adapt to
light/dark modes and avoid contrast issues.
- Around line 54-56: The `.Node` CSS rule in ObjectInspector.module.css is empty
and should be removed to avoid dead code; search the codebase for the ".Node"
selector or the symbol "Node" import usage from ObjectInspector.module.css
(e.g., className={styles.Node}) and if no JavaScript/JSX references exist delete
the `.Node { /* ... */ }` block, otherwise either add the intended styles or
keep a short comment explaining it’s used for JS targeting to prevent accidental
removal.
In `@packages/editors/src/components/object-inspector/ObjectInspector.tsx`:
- Around line 114-121: The tree row is clickable but not keyboard-accessible;
update the div in ObjectInspector.tsx (the element using
className={cn(styles.Row, isHighlighted && styles.RowHighlight)} and
onClick={toggle}) to be focusable and handle keyboard activation: add a tabIndex
(e.g., tabIndex={isExpandable && !isCircular ? 0 : undefined}) and an onKeyDown
handler that listens for Enter and Space and calls toggle (preventDefault for
Space), and ensure the handler only activates when isExpandable && !isCircular
to match the existing aria-expanded logic.
- Around line 173-185: handleCopy currently uses JSON.stringify and toYaml which
fail for cyclic/shared references; replace this with an ancestor-based
serializer (the same traversal logic used by the tree renderer) that tracks the
current ancestor stack instead of a global seen set and emits stable reference
markers (or inlines non-cyclic shared siblings correctly) so copying/exporting
works for cyclic and shared structures; update the serialization used in
handleCopy and the equivalent export path around lines 243-280 to call that new
ancestor-based serializer for both JSON and YAML outputs (reuse the tree
renderer's traversal function or extract its traversal into a shared utility and
invoke it from handleCopy and the other export handler).
In `@packages/editors/src/components/terminal/Terminal.stories.tsx`:
- Around line 450-455: The "Save Buffer" button is missing an explicit type
which can cause it to act as a submit in forms; update the JSX for the button
that calls setBuffer(ref.current?.getBufferContent() ?? '') (the Save Buffer
button) to include type="button" so it doesn't trigger form submission—locate
the button element in Terminal.stories.tsx and add the type attribute to the
existing props.
- Around line 331-348: The three search control button elements in
Terminal.stories.tsx (the buttons that call ref.current?.findNext(searchTerm),
ref.current?.findPrevious(searchTerm), and ref.current?.clearSearch()) need an
explicit type attribute to prevent them from acting as submit buttons in forms;
add type="button" to each of those button elements (the Find Next, Find
Previous, and Clear buttons) so their onClick handlers only trigger the intended
search actions.
In `@packages/editors/src/components/terminal/Terminal.test.tsx`:
- Around line 160-164: The helper waitForInit currently uses a fixed 10ms
setTimeout which is flaky; replace it by using testing-library's polling
utilities—either call vi.waitFor with an assertion that reflects the initialized
state (e.g., expect(...).toBeInTheDocument()) or use findBy* queries (e.g.,
await screen.findByText/Role) inside tests instead of waitForInit; update any
tests that call waitForInit to await the new waitFor/findBy-based condition and
remove the arbitrary setTimeout and act wrapper from the waitForInit function.
In `@packages/editors/src/components/terminal/Terminal.tsx`:
- Line 698: Add documentation to the Terminal component explaining which props
require a remount to take effect: update the component JSDoc (above the Terminal
function or its exported component) to include a `@note` listing the props that
only apply on initial mount (renderer, linkHandling, customKeyEventHandler,
fontFamily, fontWeight, fontWeightBold, convertEol, allowTransparency,
macOptionIsMeta, macOptionClickForcesSelection, disableStdin, rows, cols,
screenReaderMode, minimumContrastRatio, drawBoldTextInBrightColors). Ensure the
note references the useEffect with an empty dependency array in Terminal (the
effect that currently uses [] to avoid re-renders) so readers understand why
these props don’t update dynamically.
- Around line 641-644: The code currently assigns a cleanup function to the
container DOM node via the __windowResizeCleanup property which is an
anti-pattern; instead create a component-level ref (e.g.,
windowResizeCleanupRef) to hold the cleanup function, set
windowResizeCleanupRef.current = () => window.removeEventListener('resize',
handleWindowResize) when initializing (use the same container variable and
handleWindowResize), and call windowResizeCleanupRef.current() during unmount or
when replacing the container; remove any assignment to (container as
HTMLDivElement).__windowResizeCleanup and ensure the ref is declared in the
Terminal component so React manages lifecycle cleanly.
In `@packages/editors/src/schemas/EditorSchemaRegistry.ts`:
- Around line 245-254: The _notify() method currently swallows exceptions from
listener callbacks; update its catch block to log the thrown error (including
which listener or count/context) using the existing logging helper (log) so
listener failures are recorded without rethrowing; reference _notify, _listeners
and log when adding a descriptive error log message inside the catch to aid
production troubleshooting.
In `@packages/editors/src/setup/setupMonacoWorkers.ts`:
- Around line 96-106: The catch in the configureMonacoYaml block swallows
initialization errors making YAML setup failures invisible (so
editorSchemas._setYamlHandle is never called and later applyYamlSchemas() fails
silently); update the surrounding setup function (e.g., setupMonacoWorkers) to
propagate failure: either rethrow the caught error inside the catch after
logging (so callers see the exception) or change the function signature to
return a boolean/result and on error return a failure value; ensure
configureMonacoYaml errors are surfaced and that editorSchemas._setYamlHandle is
only skipped when callers can detect the failure.
- Around line 41-113: Move all side-effect code into the exported
setupMonacoWorkers() and make it idempotent: add a module-scoped guard (e.g.,
monacoWorkersInitialized) and return early if already initialized; inside that
function, create the getWorker implementation and merge it into any existing
self.MonacoEnvironment (do not assign self.MonacoEnvironment = {...} — instead
copy existing props and set/override getWorker), then run the
configureMonacoYaml(monaco, ...) call and call editorSchemas._setYamlHandle as
before; finally, surface YAML configuration failures instead of silently hiding
them behind warn() by rethrowing the error or returning a failed result so
callers can handle it. Ensure references: setupMonacoWorkers,
self.MonacoEnvironment, getWorker, configureMonacoYaml, and
editorSchemas._setYamlHandle are updated accordingly.
In `@packages/editors/src/themes/index.ts`:
- Around line 1-4: Export the ThemeMode type so consumers can import it: update
useEditorTheme.ts to export the ThemeMode type declaration (ensure the type is
exported, e.g., export type ThemeMode = ...) and then re-export it from this
barrel in packages/editors/src/themes/index.ts by adding a type re-export for
ThemeMode alongside existing exports of useEditorTheme and getComputedToken;
reference the ThemeMode symbol and the useEditorTheme module when making the
changes.
In `@packages/editors/src/themes/monaco.ts`:
- Line 3: Extract the duplicated ThemeMode union type into a single shared type
and update both usages to import it: create a new types file (e.g., types.ts)
that exports ThemeMode, replace the local ThemeMode declarations in monaco.ts
and useEditorTheme.ts with imports from that shared types.ts, and re-export
ThemeMode from the package index so consumers can import it from the package
root; update any import paths in files referencing ThemeMode accordingly to
ensure a single source of truth.
In `@packages/editors/src/themes/xterm.ts`:
- Around line 28-30: The token() helper currently returns an empty string when a
CSS token is missing, which can break required xterm theme properties; update
token(name: string) or its callsites (e.g., where background, foreground, and
ANSI colors are set) to provide and validate sensible fallbacks for required
properties: ensure token accepts an optional default value or that callers pass
a default (for example for background, foreground and ANSI entries) and coerce
missing/empty results to those defaults, and add a small validation/assertion
around the theme assembly to surface missing critical tokens (using the function
name token and the theme property names background, foreground, and ANSI colors
to locate changes).
In `@packages/editors/vite.config.ts`:
- Line 1: The alias path is being converted using URL.pathname which preserves
percent-encoding and yields URL-style paths that can break on Windows and with
special characters; update the Vite config to import fileURLToPath from 'url'
and use fileURLToPath(...) instead of URL.pathname when constructing the alias
target (the code around defineConfig / the alias entry), so the alias resolves
to a proper platform-native filesystem path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3c83107-2720-49be-948a-bc4e71d43799
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (64)
packages/base-ui/src/components/command-list/CommandList.module.csspackages/base-ui/src/components/separator/Separator.module.csspackages/base-ui/src/theme/styles.csspackages/editors/.storybook/main.tspackages/editors/.storybook/preview.tsxpackages/editors/docs/MONACO_YAML_COMPAT.mdpackages/editors/package.jsonpackages/editors/src/components/code-editor/CodeEditor.module.csspackages/editors/src/components/code-editor/CodeEditor.stories.tsxpackages/editors/src/components/code-editor/CodeEditor.test.tsxpackages/editors/src/components/code-editor/CodeEditor.tsxpackages/editors/src/components/code-editor/index.tspackages/editors/src/components/command-palette/CommandPalette.module.csspackages/editors/src/components/command-palette/CommandPalette.stories.tsxpackages/editors/src/components/command-palette/CommandPalette.test.tsxpackages/editors/src/components/command-palette/CommandPalette.tsxpackages/editors/src/components/command-palette/index.tspackages/editors/src/components/diff-viewer/DiffViewer.module.csspackages/editors/src/components/diff-viewer/DiffViewer.stories.tsxpackages/editors/src/components/diff-viewer/DiffViewer.test.tsxpackages/editors/src/components/diff-viewer/DiffViewer.tsxpackages/editors/src/components/diff-viewer/index.tspackages/editors/src/components/index.tspackages/editors/src/components/markdown-preview/MarkdownPreview.module.csspackages/editors/src/components/markdown-preview/MarkdownPreview.stories.tsxpackages/editors/src/components/markdown-preview/MarkdownPreview.test.tsxpackages/editors/src/components/markdown-preview/MarkdownPreview.tsxpackages/editors/src/components/markdown-preview/index.tspackages/editors/src/components/object-inspector/ObjectInspector.module.csspackages/editors/src/components/object-inspector/ObjectInspector.stories.tsxpackages/editors/src/components/object-inspector/ObjectInspector.test.tsxpackages/editors/src/components/object-inspector/ObjectInspector.tsxpackages/editors/src/components/object-inspector/index.tspackages/editors/src/components/terminal/Terminal.module.csspackages/editors/src/components/terminal/Terminal.stories.tsxpackages/editors/src/components/terminal/Terminal.test.tsxpackages/editors/src/components/terminal/Terminal.tsxpackages/editors/src/components/terminal/index.tspackages/editors/src/css.d.tspackages/editors/src/index.tspackages/editors/src/schemas/EditorSchemaRegistry.tspackages/editors/src/schemas/index.tspackages/editors/src/schemas/kubernetes/configmap-v1.jsonpackages/editors/src/schemas/kubernetes/deployment-apps-v1.jsonpackages/editors/src/schemas/kubernetes/index.tspackages/editors/src/schemas/kubernetes/ingress-networking-v1.jsonpackages/editors/src/schemas/kubernetes/pod-v1.jsonpackages/editors/src/schemas/kubernetes/service-v1.jsonpackages/editors/src/setup/css.worker.tspackages/editors/src/setup/editor.worker.tspackages/editors/src/setup/html.worker.tspackages/editors/src/setup/index.tspackages/editors/src/setup/json.worker.tspackages/editors/src/setup/setupMonacoWorkers.tspackages/editors/src/setup/ts.worker.tspackages/editors/src/setup/yaml.worker.tspackages/editors/src/themes/index.tspackages/editors/src/themes/monaco.tspackages/editors/src/themes/useEditorTheme.tspackages/editors/src/themes/xterm.tspackages/editors/src/utils/language.tspackages/editors/tsconfig.jsonpackages/editors/vite.config.tspackages/editors/vitest.setup.ts
Comment on lines
+1
to
+4
| export { useEditorTheme, getComputedToken } from './useEditorTheme'; | ||
| export { buildMonacoTheme, OV_MONACO_THEME } from './monaco'; | ||
| export type { XtermThemeData } from './xterm'; | ||
| export { buildXtermTheme } from './xterm'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider exporting ThemeMode type for consumers.
Consumers of useEditorTheme or buildMonacoTheme may need the ThemeMode type for type annotations. Consider adding:
-export { useEditorTheme, getComputedToken } from './useEditorTheme';
+export { useEditorTheme, getComputedToken, type ThemeMode } from './useEditorTheme';This requires first exporting ThemeMode from useEditorTheme.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/editors/src/themes/index.ts` around lines 1 - 4, Export the
ThemeMode type so consumers can import it: update useEditorTheme.ts to export
the ThemeMode type declaration (ensure the type is exported, e.g., export type
ThemeMode = ...) and then re-export it from this barrel in
packages/editors/src/themes/index.ts by adding a type re-export for ThemeMode
alongside existing exports of useEditorTheme and getComputedToken; reference the
ThemeMode symbol and the useEditorTheme module when making the changes.
| @@ -0,0 +1,177 @@ | |||
| import { getComputedToken } from './useEditorTheme'; | |||
|
|
|||
| type ThemeMode = 'dark' | 'light' | 'high-contrast-dark' | 'high-contrast-light'; | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting ThemeMode to avoid duplication.
ThemeMode is defined identically in both monaco.ts and useEditorTheme.ts. Extract it to a shared location (e.g., types.ts) and re-export from the index to ensure consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/editors/src/themes/monaco.ts` at line 3, Extract the duplicated
ThemeMode union type into a single shared type and update both usages to import
it: create a new types file (e.g., types.ts) that exports ThemeMode, replace the
local ThemeMode declarations in monaco.ts and useEditorTheme.ts with imports
from that shared types.ts, and re-export ThemeMode from the package index so
consumers can import it from the package root; update any import paths in files
referencing ThemeMode accordingly to ensure a single source of truth.
- Add theme token overrides for editor/terminal/syntax in base-ui styles - Extract shared ThemeMode type, add xterm theme fallbacks - Add idempotency guard and error surfacing in setupMonacoWorkers - Replace wildcard exports with explicit named exports - Improve accessibility, keyboard handling, and CSS variable usage - Fix DiffViewer language reset, CodeEditor model disposal - Add button type attributes, fix story code fence escaping - Harden ObjectInspector with keyboard nav and circular ref safety - Use fileURLToPath in vite config, surface schema registry errors
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (4)
packages/editors/src/components/diff-viewer/DiffViewer.tsx (1)
47-60:⚠️ Potential issue | 🟠 MajorKeep Monaco singleton defaults out of this component.
typescriptDefaults/javascriptDefaultsare global to the Monaco instance. Mounting oneDiffViewernow disables diagnostics and rewrites compiler options for every Monaco editor on the page, and unmount never restores the previous state. If the goal is only a quiet diff surface, prefer per-editor options such asrenderValidationDecorations: 'off'instead of mutating shared defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editors/src/components/diff-viewer/DiffViewer.tsx` around lines 47 - 60, The component currently mutates global Monaco defaults (monaco.languages.typescript.typescriptDefaults / javascriptDefaults via setDiagnosticsOptions and setCompilerOptions) which affects all editors; remove these calls and instead configure the diff editor instance only: stop using diagOpts and calls to setDiagnosticsOptions/setCompilerOptions inside DiffViewer.tsx, and pass per-editor options (e.g. renderValidationDecorations: 'off', minimap: { enabled: false }, etc.) to the diff editor creation or the Editor/MonacoReact props so validation is disabled only for this instance; if any compiler-level global changes are truly required, move them to a single app initialization path outside the component rather than here.packages/editors/src/components/diff-viewer/DiffViewer.test.tsx (1)
18-45:⚠️ Potential issue | 🟡 MinorWire
getModel()to the actualcreateModel()mocks.The mock diff editor returns ad hoc model objects instead of the instances created by
monaco.editor.createModel(). That means the unmount test still passes even if the real models leaked, becauseeditor.setModel()is never reflected bygetModel().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editors/src/components/diff-viewer/DiffViewer.test.tsx` around lines 18 - 45, The test mocks createModel and getModel separately which causes getModel() to return null/ad hoc objects rather than the actual model instances created by createModel; update the mocks so createModel stores the created model object (for example in a module-scoped variable) and have getModel() return that stored instance, and also update createDiffEditor/mockDiffEditorInstance.getModel to return { original: storedOriginalModel, modified: storedModifiedModel } (or have createDiffEditor call createModel and assign those to the stored variables) so editor.setModel() and getModel() reflect the same mocked model instances.packages/editors/src/components/command-palette/CommandPalette.test.tsx (1)
162-168:⚠️ Potential issue | 🟡 MinorRemove unused
_itemKeyparameter.The
_itemKeyparameter is destructured but never used. Remove it from the destructuring pattern to satisfy the linter.🛠️ Proposed fix
- function MockItem({ children, itemKey: _itemKey, ...props }: Record<string, unknown>) { + function MockItem({ children, itemKey: _, ...props }: Record<string, unknown>) {Or if the linter still complains about
_:- function MockItem({ children, itemKey: _itemKey, ...props }: Record<string, unknown>) { + function MockItem({ children, ...props }: Record<string, unknown>) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editors/src/components/command-palette/CommandPalette.test.tsx` around lines 162 - 168, The MockItem component currently destructures an unused parameter `_itemKey`; remove `_itemKey` from the props destructuring in the MockItem function signature so it no longer extracts that unused prop (i.e., change the parameter pattern used by MockItem to not include `_itemKey`) to satisfy the linter while keeping the rest of the props spread and children handling intact.packages/editors/src/components/object-inspector/ObjectInspector.module.css (1)
105-120:⚠️ Potential issue | 🟡 MinorUse theme-neutral fallbacks for typed values.
These fallback chains still end in hardcoded dark-theme hex colors. Any host missing the
--ov-syntax-*/--ov-color-fg-*tokens will render low-contrast values in light mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editors/src/components/object-inspector/ObjectInspector.module.css` around lines 105 - 120, The CSS rules for .Value[data-type='string'], .Value[data-type='number'], .Value[data-type='boolean'], and .Value[data-type='null']/undefined use hardcoded hex fallbacks which break in light themes; update each color fallback chain to a theme‑neutral token (for example use var(--ov-color-fg, currentColor) or currentColor as the final fallback) so the declarations become something like var(--ov-syntax-*, var(--ov-color-fg, currentColor)) and remove the direct hex literals to ensure proper contrast when host tokens are missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/editors/src/components/code-editor/CodeEditor.stories.tsx`:
- Around line 384-389: The cleanup currently calls editorSchemas.clear() which
wipes the shared singleton and breaks other stories; instead record the specific
URIs you register in this story (e.g., collect them when calling
editorSchemas.register(...)) and in the useEffect cleanup only unregister those
URIs (or snapshot the registry before the story mounts and restore that snapshot
on unmount). Update the useEffect return to remove only the story-owned entries
from the editorSchemas registry rather than calling editorSchemas.clear(), and
apply the same change to the other cleanup at the indicated block (lines
~409-412) so Unload All doesn't clear the global registry.
- Around line 31-35: The language control options in CodeEditor.stories.tsx
don't cover all story states (e.g., 'markdown' and the auto-detection case used
as undefined), so update the argTypes for the language control to include
'markdown' and an explicit "auto" (or "undefined") option, then update the
affected stories (NoLineNumbers and LanguageDetection) to use "markdown" or
"auto" respectively and map "auto" back to undefined when passing props to
CodeEditor; specifically modify the language argTypes options array and adjust
the LanguageDetection and NoLineNumbers story args to convert the "auto"
sentinel into undefined before rendering.
- Around line 597-627: The Monaco completion provider registered in handleMount
via m.languages.registerCompletionItemProvider is never disposed, causing
duplicate suggestions on remount; capture the returned IDisposable when calling
registerCompletionItemProvider (e.g., const disposable =
m.languages.registerCompletionItemProvider(...)) and store it (component state
or a ref) and ensure you call disposable.dispose() during cleanup/unmount (or in
the story's teardown effect) to unregister the provider and prevent suggestion
accumulation across remounts.
In `@packages/editors/src/components/code-editor/CodeEditor.test.tsx`:
- Around line 135-152: The two JSON tests ("pretty-prints JSON content" and
"passes through invalid JSON unchanged") assert mockCreateModel with only two
args, which is inconsistent with other tests that expect a third URI arg; update
those expectations to include the same third argument shape used elsewhere (the
URI/uri parameter passed to createModel) — e.g., change the assertions in
CodeEditor.test.tsx to assert mockCreateModel was called with
(formattedOrBadJson, 'json', <same third-argument/matcher used in "detects
language from filename">) so all tests consistently check the three-argument
call to createModel.
In `@packages/editors/src/components/code-editor/CodeEditor.tsx`:
- Around line 343-347: The model creation path uses getOrCreateModel with
resolvedLanguage even when syntaxHighlighting is false, so filename-driven model
swaps can reintroduce highlighting; update every model-creation call (e.g., the
call at getOrCreateModel(...) around displayValue/filename and the other places
mentioned) to use the current syntaxHighlighting guard: pass syntaxHighlighting
? resolvedLanguage : 'plaintext' (derive the language from the current
syntaxHighlighting state/value rather than from filename or a stale
resolvedLanguage) so replacement models remain plaintext when highlighting is
disabled and consistent across filename or language-change effects.
- Around line 216-231: getOrCreateModel currently returns the global monaco
model for a URI but unmount logic disposes it unconditionally; update
getOrCreateModel and the editor lifecycle so that model ownership is tracked
(e.g., a Map keyed by uri.toString() storing either an owner id or a refcount)
and only decrement the refcount / dispose the underlying
monaco.editor.ITextModel in the component teardown when the count reaches zero
(or when the recorded owner matches the component that created it).
Specifically, modify getOrCreateModel, the code paths that call
monaco.editor.getModel and monaco.editor.createModel, and the
unmount/filename-swap disposal logic to increment the refcount on acquire and
decrement-and-conditional-dispose on release; apply the same ownership/refcount
approach to the other occurrences noted (around the 376-388 and 458-462 regions)
so shared URIs are never disposed by an editor that didn’t create/acquire them.
- Around line 324-333: The current code mutates global Monaco language defaults
via monaco.languages.typescript.typescriptDefaults.setDiagnosticsOptions and
monaco.languages.typescript.javascriptDefaults.setDiagnosticsOptions (causing
"last editor wins" and no reactive updates); instead remove calls to
setDiagnosticsOptions and implement diagnostics on a per-editor-model basis:
when diagnostics is true, obtain diagnostics for this editor's model (use
monaco.languages.typescript.getTypeScriptWorker / getJavaScriptWorker or
existing diagnostics provider) and call monaco.editor.setModelMarkers(model,
owner, markers) to show problems for only that model; when diagnostics is false,
clear markers for this model via setModelMarkers(model, owner, []). Also add a
useEffect that watches the diagnostics prop (fixing the missing reactive updates
mentioned near the sync effect at lines ~423–436) to toggle/refresh markers for
this editor without touching the global defaults.
In `@packages/editors/src/components/command-palette/CommandPalette.stories.tsx`:
- Around line 26-40: The Storybook control for "open" is disconnected because
the stories override local state instead of using the meta args; update the
stories in CommandPalette.stories.tsx to either read and use args.open and
args.onClose (so the component's open state is driven by meta args) or
remove/disable the "open" control in argTypes for those stateful stories;
specifically, modify the story implementations that currently manage their own
open state to accept the Story args and pass args.open and args.onClose through
to <CommandPalette>, or change argTypes.open to { table: { disable: true } } to
disable the control.
In `@packages/editors/src/components/command-palette/CommandPalette.tsx`:
- Around line 46-54: Remove the obsolete ESLint disable comment referencing
jsx-a11y/no-static-element-interactions in CommandPalette.tsx; locate the
overlay element using the div with className styles.Overlay and its handlers
(onClick={onClose}, onKeyDown) and either delete the "//
eslint-disable-next-line jsx-a11y/no-static-element-interactions" line or
instead enable the rule by installing/configuring eslint-plugin-jsx-a11y in the
project so the rule exists and the lint suppression is valid.
In
`@packages/editors/src/components/markdown-preview/MarkdownPreview.stories.tsx`:
- Around line 454-456: The README sample in MarkdownPreview.stories.tsx contains
a relative link "[LICENSE](LICENSE)" that 404s in Storybook; update the sample
content where the string "## License\n\nMIT — see [LICENSE](LICENSE) for
details." is defined to use a resolvable URL (e.g., the absolute repository
license URL) or replace the link with plain text "LICENSE" so it resolves
correctly in the Storybook preview.
In `@packages/editors/src/components/markdown-preview/MarkdownPreview.tsx`:
- Around line 37-38: The module-level detailsCounter causes SSR hydration ID
mismatches; remove detailsCounter and move ID generation into React component
scope using React.useId() (or a deterministic hash from content position) so
server/client IDs match. Concretely: stop using the module-level detailsCounter
in the markdownComponents details renderer, create a Details/DetailsRenderer
component inside MarkdownPreview that calls useId() (or computes a content-based
id) and use that component from markdownComponents by turning markdownComponents
into a factory or by injecting the renderer from MarkdownPreview; update
references to detailsCounter and ensure the id attribute for the
<details>/<summary> uses the useId()-derived or hashed value.
- Around line 128-139: The custom renderer for inputs in MarkdownPreview (the
input: ({ type, checked, disabled }: MdProps) function) currently returns a raw
<input type={type} /> for any non-checkbox type, which can produce unexpected
elements; change the fallback to defensively handle unsupported types—either
return null for any type not explicitly supported (e.g., only 'checkbox'
allowed) or sanitize/validate `type` against a whitelist (e.g., 'text', 'radio'
if intended) before rendering; update the input renderer so unsupported/invalid
`type` values do not produce arbitrary inputs and document the accepted types in
MdProps.
In `@packages/editors/src/components/object-inspector/ObjectInspector.module.css`:
- Around line 20-34: The .SearchInput rule currently sets outline: none which
removes keyboard focus indication; add a :focus-visible (and optionally :focus)
style for .SearchInput to restore an accessible visible focus state (e.g., a
distinct outline, box-shadow, or border color) so keyboard users can see focus;
update the CSS by keeping .SearchInput but adding a .SearchInput:focus-visible
selector that applies a clear focus style (and ensure it contrasts with the
background) to replace outline: none.
In `@packages/editors/src/components/object-inspector/ObjectInspector.tsx`:
- Around line 86-91: The current useMemo returns an object that only implements
has() but is cast to WeakSet<object> (childAncestors), which will break if
callers use add/delete; update the implementation so childAncestors is either a
real WeakSet or accurately typed: if you need full WeakSet behavior create a new
WeakSet based on ancestors and call .add(value) when appropriate (e.g., const ws
= new WeakSet(); copy ancestors entries and ws.add(value); return ws), otherwise
change the return type to a custom interface like { has(v: object): boolean }
and stop casting to WeakSet; locate the logic in the useMemo that references
isExpandable, isObjectRef, isCircular, ancestors and modify it accordingly.
In `@packages/editors/src/components/terminal/Terminal.tsx`:
- Around line 63-67: The props onSignal and onClose are stored in callbacksRef
but never invoked; update the component to call
callbacksRef.current.onSignal(signal, payload) wherever the terminal/session
emits a signal (e.g., in the socket or pty message handler that receives signal
events or in the function currently handling "signal" messages) and call
callbacksRef.current.onClose(code) when the session/connection closes (e.g., in
the socket 'close'/'disconnect' handler or terminal exit handler). Use optional
chaining (callbacksRef.current?.onSignal(...),
callbacksRef.current?.onClose(...)) and keep existing cleanup logic intact so
these callbacks fire reliably for consumers.
- Around line 440-446: The init() async path captures reactive props into
termOptions up-front causing stale values if those props change before
termRef.current is set; update the init logic in the useEffect that creates the
terminal (the init() function and termOptions usage) to either (a) read current
reactive values from dedicated refs (e.g., fontSizeRef, cursorBlinkRef,
cursorStyleRef, scrollbackRef, lineHeightRef, letterSpacingRef) at the moment
right before applying options, or (b) after terminal creation call
terminal?.options = { ...current values } or termRef.current?.setOption /
termRef.current?.setOptions to reapply those reactive props (and ensure the
separate update effects still run but will no-op safely). Locate init(),
termOptions, and termRef in the component and implement one of these approaches
so reactive props are applied after async awaits complete.
In `@packages/editors/src/schemas/EditorSchemaRegistry.ts`:
- Around line 62-65: The globMatch function builds a RegExp from the pattern,
which can be abused by patterns with many or nested '*' to cause ReDoS; modify
globMatch to defend-in-depth by either (A) sanitizing the incoming pattern:
collapse consecutive '*' into a single '*', enforce a max pattern length and a
max wildcard count (e.g., reject or return false if pattern.length > 200 or
wildcardCount > 10) before constructing the RegExp, or (B) replace the RegExp
approach with a simple non-regex iterative glob matcher (or switch to a vetted
library like picomatch) that matches pattern against str; update references to
pattern and any callers deriving fileMatch in EditorSchemaRegistry to use the
new safe globMatch behavior.
---
Duplicate comments:
In `@packages/editors/src/components/command-palette/CommandPalette.test.tsx`:
- Around line 162-168: The MockItem component currently destructures an unused
parameter `_itemKey`; remove `_itemKey` from the props destructuring in the
MockItem function signature so it no longer extracts that unused prop (i.e.,
change the parameter pattern used by MockItem to not include `_itemKey`) to
satisfy the linter while keeping the rest of the props spread and children
handling intact.
In `@packages/editors/src/components/diff-viewer/DiffViewer.test.tsx`:
- Around line 18-45: The test mocks createModel and getModel separately which
causes getModel() to return null/ad hoc objects rather than the actual model
instances created by createModel; update the mocks so createModel stores the
created model object (for example in a module-scoped variable) and have
getModel() return that stored instance, and also update
createDiffEditor/mockDiffEditorInstance.getModel to return { original:
storedOriginalModel, modified: storedModifiedModel } (or have createDiffEditor
call createModel and assign those to the stored variables) so editor.setModel()
and getModel() reflect the same mocked model instances.
In `@packages/editors/src/components/diff-viewer/DiffViewer.tsx`:
- Around line 47-60: The component currently mutates global Monaco defaults
(monaco.languages.typescript.typescriptDefaults / javascriptDefaults via
setDiagnosticsOptions and setCompilerOptions) which affects all editors; remove
these calls and instead configure the diff editor instance only: stop using
diagOpts and calls to setDiagnosticsOptions/setCompilerOptions inside
DiffViewer.tsx, and pass per-editor options (e.g. renderValidationDecorations:
'off', minimap: { enabled: false }, etc.) to the diff editor creation or the
Editor/MonacoReact props so validation is disabled only for this instance; if
any compiler-level global changes are truly required, move them to a single app
initialization path outside the component rather than here.
In `@packages/editors/src/components/object-inspector/ObjectInspector.module.css`:
- Around line 105-120: The CSS rules for .Value[data-type='string'],
.Value[data-type='number'], .Value[data-type='boolean'], and
.Value[data-type='null']/undefined use hardcoded hex fallbacks which break in
light themes; update each color fallback chain to a theme‑neutral token (for
example use var(--ov-color-fg, currentColor) or currentColor as the final
fallback) so the declarations become something like var(--ov-syntax-*,
var(--ov-color-fg, currentColor)) and remove the direct hex literals to ensure
proper contrast when host tokens are missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0de8dd26-4744-4a12-ad4f-7d0f657a94b5
📒 Files selected for processing (26)
packages/base-ui/src/theme/styles.csspackages/editors/docs/MONACO_YAML_COMPAT.mdpackages/editors/src/components/code-editor/CodeEditor.stories.tsxpackages/editors/src/components/code-editor/CodeEditor.test.tsxpackages/editors/src/components/code-editor/CodeEditor.tsxpackages/editors/src/components/command-palette/CommandPalette.module.csspackages/editors/src/components/command-palette/CommandPalette.stories.tsxpackages/editors/src/components/command-palette/CommandPalette.test.tsxpackages/editors/src/components/command-palette/CommandPalette.tsxpackages/editors/src/components/diff-viewer/DiffViewer.test.tsxpackages/editors/src/components/diff-viewer/DiffViewer.tsxpackages/editors/src/components/index.tspackages/editors/src/components/markdown-preview/MarkdownPreview.stories.tsxpackages/editors/src/components/markdown-preview/MarkdownPreview.tsxpackages/editors/src/components/object-inspector/ObjectInspector.module.csspackages/editors/src/components/object-inspector/ObjectInspector.tsxpackages/editors/src/components/terminal/Terminal.stories.tsxpackages/editors/src/components/terminal/Terminal.tsxpackages/editors/src/schemas/EditorSchemaRegistry.tspackages/editors/src/setup/setupMonacoWorkers.tspackages/editors/src/themes/index.tspackages/editors/src/themes/monaco.tspackages/editors/src/themes/types.tspackages/editors/src/themes/useEditorTheme.tspackages/editors/src/themes/xterm.tspackages/editors/vite.config.ts
Comment on lines
+135
to
+152
| it('pretty-prints JSON content', () => { | ||
| const json = '{"a":1}'; | ||
| render(<CodeEditor value={json} language="json" />); | ||
| const formatted = JSON.stringify({ a: 1 }, null, 2); | ||
| expect(mockCreateModel).toHaveBeenCalledWith( | ||
| formatted, | ||
| 'json', | ||
| ); | ||
| }); | ||
|
|
||
| it('passes through invalid JSON unchanged', () => { | ||
| const badJson = '{not valid json}'; | ||
| render(<CodeEditor value={badJson} language="json" />); | ||
| expect(mockCreateModel).toHaveBeenCalledWith( | ||
| badJson, | ||
| 'json', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent createModel assertion arguments.
The JSON tests (lines 139-142 and 148-151) assert createModel with only 2 arguments, while other tests like detects language from filename (lines 111-115) assert 3 arguments including the URI. This inconsistency may cause false positives if the implementation changes.
♻️ Consider aligning assertions
it('pretty-prints JSON content', () => {
const json = '{"a":1}';
render(<CodeEditor value={json} language="json" />);
const formatted = JSON.stringify({ a: 1 }, null, 2);
expect(mockCreateModel).toHaveBeenCalledWith(
formatted,
'json',
+ expect.anything(),
);
});
it('passes through invalid JSON unchanged', () => {
const badJson = '{not valid json}';
render(<CodeEditor value={badJson} language="json" />);
expect(mockCreateModel).toHaveBeenCalledWith(
badJson,
'json',
+ expect.anything(),
);
});📝 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.
Suggested change
| it('pretty-prints JSON content', () => { | |
| const json = '{"a":1}'; | |
| render(<CodeEditor value={json} language="json" />); | |
| const formatted = JSON.stringify({ a: 1 }, null, 2); | |
| expect(mockCreateModel).toHaveBeenCalledWith( | |
| formatted, | |
| 'json', | |
| ); | |
| }); | |
| it('passes through invalid JSON unchanged', () => { | |
| const badJson = '{not valid json}'; | |
| render(<CodeEditor value={badJson} language="json" />); | |
| expect(mockCreateModel).toHaveBeenCalledWith( | |
| badJson, | |
| 'json', | |
| ); | |
| }); | |
| it('pretty-prints JSON content', () => { | |
| const json = '{"a":1}'; | |
| render(<CodeEditor value={json} language="json" />); | |
| const formatted = JSON.stringify({ a: 1 }, null, 2); | |
| expect(mockCreateModel).toHaveBeenCalledWith( | |
| formatted, | |
| 'json', | |
| expect.anything(), | |
| ); | |
| }); | |
| it('passes through invalid JSON unchanged', () => { | |
| const badJson = '{not valid json}'; | |
| render(<CodeEditor value={badJson} language="json" />); | |
| expect(mockCreateModel).toHaveBeenCalledWith( | |
| badJson, | |
| 'json', | |
| expect.anything(), | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/editors/src/components/code-editor/CodeEditor.test.tsx` around lines
135 - 152, The two JSON tests ("pretty-prints JSON content" and "passes through
invalid JSON unchanged") assert mockCreateModel with only two args, which is
inconsistent with other tests that expect a third URI arg; update those
expectations to include the same third argument shape used elsewhere (the
URI/uri parameter passed to createModel) — e.g., change the assertions in
CodeEditor.test.tsx to assert mockCreateModel was called with
(formattedOrBadJson, 'json', <same third-argument/matcher used in "detects
language from filename">) so all tests consistently check the three-argument
call to createModel.
Comment on lines
+216
to
+231
| function getOrCreateModel( | ||
| value: string, | ||
| language: string | undefined, | ||
| path: string | undefined, | ||
| ): monaco.editor.ITextModel { | ||
| if (path) { | ||
| const uri = monaco.Uri.parse(path); | ||
| log('getOrCreateModel — parsed URI:', uri.toString(), '| language:', language); | ||
| const existing = monaco.editor.getModel(uri); | ||
| if (existing) { | ||
| log(' reusing existing model'); | ||
| return existing; | ||
| } | ||
| log(' creating new model with URI'); | ||
| return monaco.editor.createModel(value, language, uri); | ||
| } |
There was a problem hiding this comment.
Shared-model reuse needs ownership tracking.
getOrCreateModel() returns the process-wide model for a URI, but unmount and filename swaps always dispose the current model. Two editors with the same filename will share buffer state, then one instance can invalidate the other's model during cleanup. Only dispose models this editor created, or add reference counting around shared URIs.
Also applies to: 376-388, 458-462
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/editors/src/components/code-editor/CodeEditor.tsx` around lines 216
- 231, getOrCreateModel currently returns the global monaco model for a URI but
unmount logic disposes it unconditionally; update getOrCreateModel and the
editor lifecycle so that model ownership is tracked (e.g., a Map keyed by
uri.toString() storing either an owner id or a refcount) and only decrement the
refcount / dispose the underlying monaco.editor.ITextModel in the component
teardown when the count reaches zero (or when the recorded owner matches the
component that created it). Specifically, modify getOrCreateModel, the code
paths that call monaco.editor.getModel and monaco.editor.createModel, and the
unmount/filename-swap disposal logic to increment the refcount on acquire and
decrement-and-conditional-dispose on release; apply the same ownership/refcount
approach to the other occurrences noted (around the 376-388 and 458-462 regions)
so shared URIs are never disposed by an editor that didn’t create/acquire them.
Comment on lines
+63
to
+67
| onSignal?: (signal: TerminalSignal, payload?: unknown) => void; | ||
| /** Called when the terminal init encounters an error (e.g., failed to load xterm). */ | ||
| onError?: (error: Error) => void; | ||
| /** Called when the terminal session closes. */ | ||
| onClose?: (code?: number) => void; |
There was a problem hiding this comment.
onSignal and onClose never fire.
These props are part of the public API, but this implementation only stores them in callbacksRef; nothing in the component ever invokes either callback. Consumers wiring exec lifecycle through them will wait forever.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/editors/src/components/terminal/Terminal.tsx` around lines 63 - 67,
The props onSignal and onClose are stored in callbacksRef but never invoked;
update the component to call callbacksRef.current.onSignal(signal, payload)
wherever the terminal/session emits a signal (e.g., in the socket or pty message
handler that receives signal events or in the function currently handling
"signal" messages) and call callbacksRef.current.onClose(code) when the
session/connection closes (e.g., in the socket 'close'/'disconnect' handler or
terminal exit handler). Use optional chaining
(callbacksRef.current?.onSignal(...), callbacksRef.current?.onClose(...)) and
keep existing cleanup logic intact so these callbacks fire reliably for
consumers.
Comment on lines
+440
to
+446
| // Initialize xterm lazily. | ||
| // @note Mount-only props (read once during init, not reactive): fontFamily, fontWeight, | ||
| // fontWeightBold, allowTransparency, convertEol, macOptionIsMeta, | ||
| // macOptionClickForcesSelection, drawBoldTextInBrightColors, disableStdin, cursorWidth, | ||
| // initialRows, initialCols, screenReaderMode, minimumContrastRatio, customKeyEventHandler, | ||
| // autoFocus, renderer, linkHandling. Changing these after mount requires remounting. | ||
| useEffect(() => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the Terminal.tsx file
find . -name "Terminal.tsx" -path "*/editors/src/components/terminal/*" | head -5Repository: omniviewdev/ui
Length of output: 113
🏁 Script executed:
# Get file size and read the relevant sections
wc -l packages/editors/src/components/terminal/Terminal.tsxRepository: omniviewdev/ui
Length of output: 115
🏁 Script executed:
# Read lines 440-460 to see the useEffect context
sed -n '430,460p' packages/editors/src/components/terminal/Terminal.tsxRepository: omniviewdev/ui
Length of output: 1341
🏁 Script executed:
# Read lines 700-770 to see the update effects mentioned
sed -n '700,770p' packages/editors/src/components/terminal/Terminal.tsxRepository: omniviewdev/ui
Length of output: 2115
🏁 Script executed:
# Read lines 440-550 to see the full init function
sed -n '440,550p' packages/editors/src/components/terminal/Terminal.tsxRepository: omniviewdev/ui
Length of output: 4451
🏁 Script executed:
# Also check if termRef.current is assigned early in init or at the end
sed -n '440,700p' packages/editors/src/components/terminal/Terminal.tsx | grep -n "termRef.current =" -A 2 -B 2Repository: omniviewdev/ui
Length of output: 343
🏁 Script executed:
# Check if there's any mechanism to re-apply props after init completes
sed -n '440,700p' packages/editors/src/components/terminal/Terminal.tsx | grep -i "useeff\|uselay\|usecallback" | head -20Repository: omniviewdev/ui
Length of output: 77
🏁 Script executed:
# Confirm dependency array for the init effect
sed -n '440,470p' packages/editors/src/components/terminal/Terminal.tsx | tail -10Repository: omniviewdev/ui
Length of output: 414
🏁 Script executed:
# Get the closing of the init useEffect to see dependency array
sed -n '680,710p' packages/editors/src/components/terminal/Terminal.tsxRepository: omniviewdev/ui
Length of output: 915
Reactive options can become stale if changed during async initialization.
The mount-only effect initializes fontSize, cursorBlink, cursorStyle, scrollback, lineHeight, and letterSpacing by capturing their values into termOptions at the start of the async init() function. However, termRef.current is not assigned until after multiple await statements (fonts, module imports, renderer setup). If any of these reactive props change during that window, the update effects attempt to run but encounter if (!term?.options) return with a null termRef.current, causing a no-op. The component then initializes with the original stale values, and since the effect dependencies remain unchanged, the effects never re-run.
Reapply reactive options after terminal initialization completes, or read current values from refs within init() instead of capturing them at the function's entry point.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/editors/src/components/terminal/Terminal.tsx` around lines 440 -
446, The init() async path captures reactive props into termOptions up-front
causing stale values if those props change before termRef.current is set; update
the init logic in the useEffect that creates the terminal (the init() function
and termOptions usage) to either (a) read current reactive values from dedicated
refs (e.g., fontSizeRef, cursorBlinkRef, cursorStyleRef, scrollbackRef,
lineHeightRef, letterSpacingRef) at the moment right before applying options, or
(b) after terminal creation call terminal?.options = { ...current values } or
termRef.current?.setOption / termRef.current?.setOptions to reapply those
reactive props (and ensure the separate update effects still run but will no-op
safely). Locate init(), termOptions, and termRef in the component and implement
one of these approaches so reactive props are applied after async awaits
complete.
- Replace global Monaco TS defaults with per-editor renderValidationDecorations - Track schema URIs per-story instead of clearing shared singleton - Dispose completion provider on unmount to prevent duplicate suggestions - Add 'auto' and 'markdown' to language control options - Fix filename swap to respect syntaxHighlighting prop - Remove DiffViewer global defaults mutation - Replace RegExp-based globMatch with iterative matcher to prevent ReDoS - Remove eslint-disable comment, add role="presentation" to overlay - Disable disconnected 'open' Storybook control - Replace module-level detailsCounter with useId-based DetailsAccordion - Return null for unsupported input types in MarkdownPreview - Fix LICENSE link in stories to plain text - Add :focus-visible to SearchInput, use currentColor fallbacks - Use honest AncestorSet interface instead of fake WeakSet cast - Clarify onSignal/onClose are consumer-wired, not internally invoked
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/editors/src/components/object-inspector/ObjectInspector.tsx (1)
255-268:⚠️ Potential issue | 🟠 MajorShared references are still serialized as cycles.
Both serializers keep a global seen set, so
{ a: shared, b: shared }copiesbas[Circular]even though there is no cycle. That still corrupts copy/export output for DAG-shaped data; use the same ancestor-stack approach as the tree renderer instead of process-wide seen state.Also applies to: 271-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editors/src/components/object-inspector/ObjectInspector.tsx` around lines 255 - 268, The current safeJsonStringify uses a global WeakSet (seen) which treats shared but non-cyclic references as cycles (e.g., {a: shared, b: shared}), corrupting DAG outputs; update safeJsonStringify (and the same logic in the related block at lines ~271-308) to use an ancestor-stack approach instead of a process-wide seen set: in the JSON replacer maintain a stack/array of the current ancestor objects, on entering an object/array push it and on exiting pop it, and only treat a value as circular if it already exists in that ancestor stack (not merely seen before), so shared nodes are serialized fully unless they are true cycles. Also ensure push/pop happens for objects and arrays and that null and non-object values are handled normally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/editors/src/components/diff-viewer/DiffViewer.tsx`:
- Around line 74-87: The original content effect updates the Monaco model
unconditionally which can reset cursor/selection; update the first useEffect
(the one handling original content that references editorRef.current, isReady,
editorRef.current.getModel()?.original?.setValue(original)) to mirror the
modified-effect guard: retrieve the original editor/model value (e.g., via
editorRef.current.getModel()?.original?.getValue() or the original editor
instance) and only call setValue(original) when the new original differs from
the current value, keeping the same early-return on !editorRef.current ||
!isReady.
In
`@packages/editors/src/components/markdown-preview/MarkdownPreview.stories.tsx`:
- Line 167: The stories in MarkdownPreview.stories.tsx embed remote images (e.g.
the Markdown line "" and similar uses at
the other ranges) which makes Storybook brittle; replace those remote image URLs
with checked-in static fixtures or inline data URIs and update the story
fixtures accordingly (import the local asset at the top of the file and use its
relative path or replace the Markdown string to reference the data URI). Locate
the Markdown strings used by the MarkdownPreview stories (search for the
placeholder image markdown and the other occurrences around the 370–373 and
402–405 ranges) and swap each remote URL for a local file import or base64 data
URI so the stories are deterministic and offline-safe.
In `@packages/editors/src/components/markdown-preview/MarkdownPreview.tsx`:
- Around line 86-90: The custom link renderer currently only accepts href and
children (a: ({ children, href }: MdProps) => ...) which drops title, aria-*,
data-* and other standard anchor props; update the renderer to destructure and
collect the rest (e.g., ({ children, href, ...props }: MdProps)) and spread
those into the Link (e.g., <Link href={href} {...props} ...>) while enforcing
target="_blank" and rel="noopener noreferrer" last so they override any incoming
values; ensure title and aria/data attributes are preserved by this change.
- Around line 56-67: The Markdown renderer collapses fenced blocks because the
code renderer only returns a CodeBlock when className matches /language-(\w+)/
(which misses languages with hyphens or pluses) and otherwise always returns
inline <Code>, losing multiline fences; update the code renderer in
MarkdownPreview.tsx (the code: ({ className, children }: MdProps) => { ... }) to
(1) use a more permissive regex like /language-([^\s]+)/ to accept names such as
"c++" or "shell-session", and (2) treat content containing a newline as a block:
if !match && String(children).includes('\n') then return <CodeBlock
language={match?.[1]} copyable>{String(children).replace(/\n$/,
'')}</CodeBlock>; otherwise return <Code>{children}</Code>. Ensure MdProps,
CodeBlock and Code references are used as in the existing file.
In `@packages/editors/src/components/object-inspector/ObjectInspector.tsx`:
- Around line 84-86: The component initializes and uses the local expanded state
(expanded, setExpanded) derived only from
isExpandable/isCircular/shouldExpand(defaultExpanded, depth), so search results
don't reveal matches inside collapsed branches; update ObjectInspector to
either: 1) derive/override expansion when a search is active by including a
query-match check in the initial state and adding an effect that sets
setExpanded(true) when the current node or any descendant matches the query (use
the existing shouldExpand/defaultExpanded/depth logic plus a prop like
queryMatches or queryActive), or 2) change rendering logic (the branch that uses
expanded to decide whether to render children around lines 150-162) to traverse
hidden subtrees when a query is active and render descendants that match even if
expanded is false; reference the expanded/setExpanded state, isExpandable,
isCircular, shouldExpand(defaultExpanded, depth), and the render-children block
to implement one of these fixes so nested matches become visible during search.
- Around line 270-308: The toYaml function currently emits unescaped plain
scalars and keys which breaks YAML for values containing colons, hashes,
leading/trailing whitespace, empty strings, or other special chars; update
toYaml to escape/quote scalars and keys: implement a helper (e.g.,
escapeYamlScalar) used when serializing strings and when emitting map keys
(references: toYaml, the Array branch map and the Object entries map where you
build `${prefix}${k}: ${serialized}` and `${prefix}${k}:\n${serialized}`); the
helper should detect safe plain scalars and otherwise return a properly quoted
double‑quoted YAML scalar that escapes backslashes, double quotes, and control
characters (and preserve newlines using the existing block style branch), and
ensure empty strings and strings with leading/trailing spaces are quoted so
round-tripping is preserved.
---
Duplicate comments:
In `@packages/editors/src/components/object-inspector/ObjectInspector.tsx`:
- Around line 255-268: The current safeJsonStringify uses a global WeakSet
(seen) which treats shared but non-cyclic references as cycles (e.g., {a:
shared, b: shared}), corrupting DAG outputs; update safeJsonStringify (and the
same logic in the related block at lines ~271-308) to use an ancestor-stack
approach instead of a process-wide seen set: in the JSON replacer maintain a
stack/array of the current ancestor objects, on entering an object/array push it
and on exiting pop it, and only treat a value as circular if it already exists
in that ancestor stack (not merely seen before), so shared nodes are serialized
fully unless they are true cycles. Also ensure push/pop happens for objects and
arrays and that null and non-object values are handled normally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14b7fd89-4f70-40d7-8251-02fe94d8186d
📒 Files selected for processing (11)
packages/editors/src/components/code-editor/CodeEditor.stories.tsxpackages/editors/src/components/code-editor/CodeEditor.tsxpackages/editors/src/components/command-palette/CommandPalette.stories.tsxpackages/editors/src/components/command-palette/CommandPalette.tsxpackages/editors/src/components/diff-viewer/DiffViewer.tsxpackages/editors/src/components/markdown-preview/MarkdownPreview.stories.tsxpackages/editors/src/components/markdown-preview/MarkdownPreview.tsxpackages/editors/src/components/object-inspector/ObjectInspector.module.csspackages/editors/src/components/object-inspector/ObjectInspector.tsxpackages/editors/src/components/terminal/Terminal.tsxpackages/editors/src/schemas/EditorSchemaRegistry.ts
- DiffViewer: Guard original content update to avoid cursor/selection reset - MarkdownPreview: Spread anchor props (title, aria-*, data-*) through Link - MarkdownPreview: Widen language regex to match hyphens/pluses (c++, shell-session) - MarkdownPreview: Treat multiline content without language as CodeBlock - MarkdownPreview stories: Replace remote images with inline SVG data URIs - ObjectInspector: Auto-expand collapsed branches when search matches descendants - ObjectInspector: Fix safeJsonStringify to use ancestor stack (DAG-safe) - ObjectInspector: Add YAML scalar quoting for special chars, colons, empty strings - ObjectInspector: Use ancestor stack in toYaml for correct shared-ref handling - DiffViewer test: Add getValue to original model mock
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/editors/src/components/diff-viewer/DiffViewer.test.tsx`:
- Around line 88-92: The test currently asserts monaco.editor.createModel was
called with the language, which overfits the implementation; instead update the
test for DiffViewer to verify the component ensures both live models end up with
the requested language by asserting calls to monaco.editor.setModelLanguage (or
by checking the created model instances have the requested language) with 'yaml'
for both original and modified models; locate usages of
monaco.editor.createModel in the test and replace the expectation with
assertions that monaco.editor.setModelLanguage was called for the created models
(or that the models returned by createModel have language 'yaml').
In
`@packages/editors/src/components/markdown-preview/MarkdownPreview.stories.tsx`:
- Around line 67-82: The example calls client.get in fetchPods but never
instantiates client; createClient is imported but not used—instantiate a client
(e.g., const client = createClient(...)) before using it so fetchPods can call
client.get; update the snippet to create the client near the top (referencing
createClient and client) or modify fetchPods to create/receive the client (e.g.,
accept a client parameter) so client.get is valid.
In `@packages/editors/src/components/markdown-preview/MarkdownPreview.tsx`:
- Line 1: Import the ReactNode type from 'react' (add ReactNode to the existing
import list) and change any uses of the unbound React namespace (e.g.
React.ReactNode[]) to just ReactNode[]; also remove the unused destructured
variable "node: _node" (or stop destructuring it) wherever it's present so the
unused variable ESLint error is resolved—look for the exact "node: _node"
destructuring and the spots that currently use React.ReactNode[] to update their
types accordingly.
- Around line 88-90: In the MarkdownPreview anchor renderer (the arrow function
at "a: ({ children, href, node: _node, ...rest }: MdProps) => ..."), remove the
unused parameter "node: _node" from the parameter list so the function signature
becomes "({ children, href, ...rest }: MdProps) => ..."; update the MdProps
usage if necessary to avoid type errors (keep rest and children/href intact) so
ESLint no-unused-vars no longer triggers.
In `@packages/editors/src/components/object-inspector/ObjectInspector.tsx`:
- Around line 58-72: subtreeMatches is repeatedly walking the same descendant
graph for every TreeNode, causing O(n²) work; change the implementation to
memoize results per-query using a WeakMap<object, boolean> (or WeakMap<object,
Map<string, boolean>> if you support multiple concurrent queries) and compute
descendant-match metadata once at the root, then read from that WeakMap inside
subtreeMatches and hasDescendantMatch instead of re-scanning children; ensure
you still respect ancestor cycles (keep using AncestorSet) and populate the
WeakMap when first visiting an object so subsequent calls for the same object
and query return the cached boolean.
- Around line 106-127: When forcing expansion during search in ObjectInspector,
snapshot the node's pre-search expanded state (e.g., save the current expanded
boolean into a new ref like preSearchExpandedRef before you first set
searchExpandedRef.current = true) so you can restore that exact state when
searchQuery becomes empty; modify the useEffect that watches
searchQuery/hasDescendantMatch to (1) on the first forced expansion store
expanded into preSearchExpandedRef and then setExpanded(true), and (2) when
clearing searchQuery and searchExpandedRef.current is true, reset expanded to
the stored preSearchExpandedRef value (and clear both refs) instead of
recomputing via shouldExpand(defaultExpanded, depth). Ensure you update
references to searchExpandedRef and include preSearchExpandedRef in the
component scope.
- Around line 326-328: Remove the unnecessary backslash escapes for the literal
`[` inside the two regex character classes: change the regex
/^[-?:,\[\]{}#&*!|>'"%@`]/.test(s) to /^[-?:,[\]{}#&*!|>'"%@`]/.test(s) (i.e.,
unescape `\[` to `[`), and change /[:\s]#|:\s|[{}\[\],]/.test(s) to
/[:\s]#|:\s|[{}\[,],]/.test(s) (unescape `\[` to `[`), ensuring both occurrences
that use .test(s) are updated so eslint/no-useless-escape is no longer
triggered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 031de3a5-7ee2-4b79-b272-d94cbabf7579
📒 Files selected for processing (5)
packages/editors/src/components/diff-viewer/DiffViewer.test.tsxpackages/editors/src/components/diff-viewer/DiffViewer.tsxpackages/editors/src/components/markdown-preview/MarkdownPreview.stories.tsxpackages/editors/src/components/markdown-preview/MarkdownPreview.tsxpackages/editors/src/components/object-inspector/ObjectInspector.tsx
Comment on lines
+88
to
+90
| a: ({ children, href, node: _node, ...rest }: MdProps) => ( | ||
| <Link {...rest} href={href} target="_blank" rel="noopener noreferrer" underline="hover"> | ||
| {children} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/editors/src/components/markdown-preview/MarkdownPreview.tsx | sed -n '80,100p'Repository: omniviewdev/ui
Length of output: 877
🏁 Script executed:
find . -maxdepth 3 -type f \( -name ".eslintrc*" -o -name "eslint.config.*" -o -name "package.json" \) | head -20Repository: omniviewdev/ui
Length of output: 156
🏁 Script executed:
cat -n eslint.config.mjs | head -100Repository: omniviewdev/ui
Length of output: 1408
🏁 Script executed:
wc -l eslint.config.mjsRepository: omniviewdev/ui
Length of output: 78
🏁 Script executed:
web_search
TypeScript ESLint no-unused-vars underscore prefix default behaviorRepository: omniviewdev/ui
Length of output: 157
🌐 Web query:
TypeScript ESLint @typescript-eslint/no-unused-vars rule underscore prefix default behavior
💡 Result:
By default, @typescript-eslint/no-unused-vars does not treat an underscore (_) prefix as special—so _foo (or _arg) is still reported as unused unless you configure ignore patterns. The rule supports options like argsIgnorePattern, varsIgnorePattern, caughtErrorsIgnorePattern, and destructuredArrayIgnorePattern to exempt names (commonly ^_). [1]
To get the “underscore means intentionally unused” behavior, configure it explicitly, e.g.:
'@typescript-eslint/no-unused-vars': ['error', {
argsIgnorePattern: '^_',
varsIgnorePattern: '^_',
caughtErrorsIgnorePattern: '^_',
destructuredArrayIgnorePattern: '^_',
}]Note: TypeScript itself (e.g., noUnusedLocals / noUnusedParameters) does ignore identifiers starting with _, but ESLint’s unused-vars rules won’t unless configured. [2]
Sources: typescript-eslint rule docs [1], ESLint/TypeScript comparison (ESLint blog) [2].
🏁 Script executed:
find packages/editors -maxdepth 2 -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \)Repository: omniviewdev/ui
Length of output: 40
🏁 Script executed:
cat -n packages/editors/src/components/markdown-preview/MarkdownPreview.tsx | sed -n '85,95p'Repository: omniviewdev/ui
Length of output: 452
Remove the unused node parameter.
The node: _node parameter is never used in the function body and will trigger a linting error with the current ESLint configuration. The underscore prefix doesn't suppress @typescript-eslint/no-unused-vars without explicit configuration, so this must be removed to pass lint checks.
Suggested fix
- a: ({ children, href, node: _node, ...rest }: MdProps) => (
+ a: ({ children, href, ...rest }: MdProps) => (🧰 Tools
🪛 ESLint
[error] 88-88: '_node' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/editors/src/components/markdown-preview/MarkdownPreview.tsx` around
lines 88 - 90, In the MarkdownPreview anchor renderer (the arrow function at "a:
({ children, href, node: _node, ...rest }: MdProps) => ..."), remove the unused
parameter "node: _node" from the parameter list so the function signature
becomes "({ children, href, ...rest }: MdProps) => ..."; update the MdProps
usage if necessary to avoid type errors (keep rest and children/href intact) so
ESLint no-unused-vars no longer triggers.
- DiffViewer test: assert language via setModelLanguage instead of createModel - MarkdownPreview: import ReactNode/ComponentType directly, handle node prop in link renderer - MarkdownPreview stories: add missing createClient() to TypeScript sample - ObjectInspector: memoize subtreeMatches with WeakMap cache, restore exact pre-search expansion state
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/editors/src/components/diff-viewer/DiffViewer.test.tsx (1)
18-45:⚠️ Potential issue | 🟠 MajorWire
getModel()to the same models returned bycreateModel().Line 18-43 still creates one model pair in
createDiffEditorand a different pair increateModel().DiffViewermanages thecreateModel()instances, so the assertions at Line 88-96 and Line 148-154 are pointed at the wrong objects and can miss regressions in language sync and cleanup.♻️ Minimal fix
+const createMockModel = () => ({ + setValue: vi.fn(), + getValue: vi.fn(() => ''), + dispose: vi.fn(), +}); + +let mockCreatedModels: Array<ReturnType<typeof createMockModel>> = []; let mockDiffEditorInstance: Record<string, unknown>; vi.mock('monaco-editor', () => ({ default: undefined, Uri: { @@ editor: { createDiffEditor: (...args: unknown[]) => { mockCreateDiffEditor(...args); - const originalModel = { - setValue: vi.fn(), - getValue: vi.fn(() => ''), - dispose: vi.fn(), - }; - const modifiedModel = { - setValue: vi.fn(), - getValue: vi.fn(() => ''), - dispose: vi.fn(), - }; mockDiffEditorInstance = { setModel: vi.fn(), - getModel: vi.fn(() => ({ original: originalModel, modified: modifiedModel })), + getModel: vi.fn(() => ({ + original: mockCreatedModels[0], + modified: mockCreatedModels[1], + })), getModifiedEditor: vi.fn(() => ({ getValue: vi.fn(() => '') })), updateOptions: vi.fn(), dispose: vi.fn(), }; return mockDiffEditorInstance; }, - createModel: vi.fn(() => ({ - setValue: vi.fn(), - getValue: vi.fn(() => ''), - dispose: vi.fn(), - })), + createModel: vi.fn(() => { + const model = createMockModel(); + mockCreatedModels.push(model); + return model; + }), @@ beforeEach(() => { vi.clearAllMocks(); + mockCreatedModels = []; }); @@ it('applies language to both models', () => { render(<DiffViewer original="" modified="" language="yaml" />); - // Language sync effect calls setModelLanguage for both models - expect(mockSetModelLanguage).toHaveBeenCalledWith( - expect.anything(), - 'yaml', - ); - // Called twice — once for original, once for modified - expect(mockSetModelLanguage).toHaveBeenCalledTimes(2); + expect(mockSetModelLanguage).toHaveBeenCalledWith(mockCreatedModels[0], 'yaml'); + expect(mockSetModelLanguage).toHaveBeenCalledWith(mockCreatedModels[1], 'yaml'); }); @@ it('disposes editor and models on unmount', () => { const { unmount } = render(<DiffViewer original="" modified="" />); - const models = (mockDiffEditorInstance.getModel as ReturnType<typeof vi.fn>)(); unmount(); - expect(models.original.dispose).toHaveBeenCalled(); - expect(models.modified.dispose).toHaveBeenCalled(); + expect(mockCreatedModels[0].dispose).toHaveBeenCalled(); + expect(mockCreatedModels[1].dispose).toHaveBeenCalled(); expect(mockDiffEditorInstance.dispose).toHaveBeenCalled(); });Also applies to: 88-96, 148-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editors/src/components/diff-viewer/DiffViewer.test.tsx` around lines 18 - 45, The test stubs create separate model objects in createDiffEditor and createModel, causing DiffViewer assertions to inspect the wrong instances; modify the mocks so createModel returns shared model instances and mockDiffEditorInstance.getModel() returns those same objects: centralize model creation into a single pair (originalModel, modifiedModel) used by createModel (the vi.fn) and by createDiffEditor’s getModel return value, and ensure any calls to getModifiedEditor/getValue and dispose map to those shared instances so language sync and cleanup assertions (around getModel, createModel, mockDiffEditorInstance) target the actual models managed by DiffViewer.packages/editors/src/components/object-inspector/ObjectInspector.tsx (1)
137-148:⚠️ Potential issue | 🟠 MajorUnwind forced expansion when the active query changes.
This effect only restores state after the query becomes empty. If a node was opened for one term and the user types a different non-empty term, it stays expanded even when
hasDescendantMatchis now false; then Line 139 can overwrite the original pre-search snapshot on the next forced reopen. Restore the saved state whenever a search-expanded node stops matching the current query, and only capturepreSearchExpandedRefon the first force-expand.♻️ Proposed fix
useEffect(() => { - if (searchQuery && hasDescendantMatch && isExpandable && !isCircular && !expanded) { - preSearchExpandedRef.current = expanded; - searchExpandedRef.current = true; - setExpanded(true); - } - // Restore pre-search state only for nodes that were force-expanded - if (!searchQuery && searchExpandedRef.current) { + const shouldForceExpand = + Boolean(searchQuery) && hasDescendantMatch && isExpandable && !isCircular; + + if (shouldForceExpand && !expanded) { + if (!searchExpandedRef.current) { + preSearchExpandedRef.current = expanded; + searchExpandedRef.current = true; + } + setExpanded(true); + return; + } + + // Restore pre-search state for nodes opened only by search. + if (!shouldForceExpand && searchExpandedRef.current) { searchExpandedRef.current = false; setExpanded(preSearchExpandedRef.current); } - }, [searchQuery, hasDescendantMatch, isExpandable, isCircular, expanded, defaultExpanded, depth]); + }, [searchQuery, hasDescendantMatch, isExpandable, isCircular, expanded]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editors/src/components/object-inspector/ObjectInspector.tsx` around lines 137 - 148, The effect currently overwrites preSearchExpandedRef and fails to restore when the active non-empty query changes; update useEffect (the block handling searchQuery, hasDescendantMatch, isExpandable, isCircular, expanded, preSearchExpandedRef, searchExpandedRef, setExpanded) so that you only snapshot preSearchExpandedRef the first time you force-open (i.e., when searchQuery truthy, hasDescendantMatch true, isExpandable true, isCircular false, and searchExpandedRef.current is false) and set searchExpandedRef.current = true, and add logic to unwind/restor e the saved state not only when searchQuery becomes empty but also when a search-expanded node stops matching the current query (i.e., when searchExpandedRef.current is true and (!searchQuery || !hasDescendantMatch) restore setExpanded(preSearchExpandedRef.current) and clear searchExpandedRef.current).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/editors/src/components/markdown-preview/MarkdownPreview.stories.tsx`:
- Around line 5-10: The data: URI image constants (PLACEHOLDER_IMG, BADGE_BUILD,
BADGE_COVERAGE, BADGE_LICENSE, BADGE_VERSION, BADGE_PRS) are stripped by
react-markdown/rehype-sanitize; replace them with checked-in SVG asset files and
use imports or relative paths instead. Add SVG fixtures into the story assets
folder, import them at top (e.g. import placeholderSvg from
'./assets/placeholder.svg') or set the constants to './assets/...svg' relative
paths, then update all usages of PLACEHOLDER_IMG and BADGE_* in the
MarkdownPreview story so markdown image links and raw <img> tags reference the
imported/relative SVGs (not data: URIs). Ensure the imported paths are included
in the package build so they resolve in storybook.
In `@packages/editors/src/components/markdown-preview/MarkdownPreview.tsx`:
- Around line 98-99: The markdown renderer maps horizontal rules via the hr
renderer to a decorative Separator which hides the thematic break from assistive
tech; update the hr renderer in MarkdownPreview.tsx so it returns a semantic,
non-decorative element (e.g., use <Separator /> without the decorative prop or
render a native <hr />) so the thematic break remains exposed to assistive
technologies; adjust any styling as needed while keeping the Separator component
name (or hr renderer) as the change target.
In `@packages/editors/src/components/object-inspector/ObjectInspector.tsx`:
- Around line 25-31: The string branch in formatValue currently interpolates raw
string content into quotes which can introduce unescaped quotes, backslashes, or
newlines; change the string handling in formatValue to return an escaped preview
instead of raw interpolation (e.g., use JSON.stringify(value) for the string
case or apply an escaping routine that replaces backslashes, double quotes,
newlines, tabs, and carriage returns with their escaped sequences) so the
displayed value is safely escaped and stays on one line; update the branch
inside function formatValue (the typeof value === 'string' case) to produce the
escaped string.
---
Duplicate comments:
In `@packages/editors/src/components/diff-viewer/DiffViewer.test.tsx`:
- Around line 18-45: The test stubs create separate model objects in
createDiffEditor and createModel, causing DiffViewer assertions to inspect the
wrong instances; modify the mocks so createModel returns shared model instances
and mockDiffEditorInstance.getModel() returns those same objects: centralize
model creation into a single pair (originalModel, modifiedModel) used by
createModel (the vi.fn) and by createDiffEditor’s getModel return value, and
ensure any calls to getModifiedEditor/getValue and dispose map to those shared
instances so language sync and cleanup assertions (around getModel, createModel,
mockDiffEditorInstance) target the actual models managed by DiffViewer.
In `@packages/editors/src/components/object-inspector/ObjectInspector.tsx`:
- Around line 137-148: The effect currently overwrites preSearchExpandedRef and
fails to restore when the active non-empty query changes; update useEffect (the
block handling searchQuery, hasDescendantMatch, isExpandable, isCircular,
expanded, preSearchExpandedRef, searchExpandedRef, setExpanded) so that you only
snapshot preSearchExpandedRef the first time you force-open (i.e., when
searchQuery truthy, hasDescendantMatch true, isExpandable true, isCircular
false, and searchExpandedRef.current is false) and set searchExpandedRef.current
= true, and add logic to unwind/restor e the saved state not only when
searchQuery becomes empty but also when a search-expanded node stops matching
the current query (i.e., when searchExpandedRef.current is true and
(!searchQuery || !hasDescendantMatch) restore
setExpanded(preSearchExpandedRef.current) and clear searchExpandedRef.current).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f6e2234-9622-4eac-9c4a-4242d0974bc5
📒 Files selected for processing (4)
packages/editors/src/components/diff-viewer/DiffViewer.test.tsxpackages/editors/src/components/markdown-preview/MarkdownPreview.stories.tsxpackages/editors/src/components/markdown-preview/MarkdownPreview.tsxpackages/editors/src/components/object-inspector/ObjectInspector.tsx
- Replace data: URI image constants with checked-in SVG asset files - Add SVG module declaration to css.d.ts - Remove decorative prop from hr Separator for accessibility - Use JSON.stringify in formatValue for safe string escaping - Fix DiffViewer test mocks to share model instances between createModel and getModel - Restore pre-search expansion state when node stops matching (not just on query clear)
This was referenced Mar 12, 2026
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@omniview/editorspackage with 6 production-grade editor components: CodeEditor, DiffViewer, Terminal, MarkdownPreview, CommandPalette, and ObjectInspectorKey Terminal features
onReadycallback for session attach timing,autoFocus,disableStdin(log viewer mode)TerminalErrorInfotype matching Go SDKStreamErrorfor structured error signalsStreamSignalenumTest plan
pnpm test— 156 tests pass across all 6 componentsSummary by CodeRabbit