Feat/parser editor#97
Open
ginccc wants to merge 4 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds a new "parser" resource type with ParserEditor UI, tests, editor/registry and API mappings, inline pipeline editing and modal dialog flow, localized UI strings across locales, and MSW mocks to return enriched parser payloads. ChangesParser Feature Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
🧹 Nitpick comments (4)
src/components/editors/__tests__/parser-editor.test.tsx (2)
489-495: ⚡ Quick winAvoid badge assertions via section
textContent; assert through explicitdata-testid.These checks depend on container text shape instead of stable test IDs. Add badge-level
data-testid(e.g., per section badge) and assert exact values there.As per coding guidelines, "Assert on
data-testidattributes in unit tests instead of relying on rendered text or DOM structure".Also applies to: 513-514
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/editors/__tests__/parser-editor.test.tsx` around lines 489 - 495, Replace assertions that inspect section.textContent with checks against explicit badge-level data-testids: add a stable data-testid on the badge elements (e.g., "parser-dictionaries-badge" and "parser-corrections-badge") in the component that renders each section, then update the test (parser-editor.test.tsx) to use screen.getByTestId("parser-dictionaries-badge") and screen.getByTestId("parser-corrections-badge") and assert their exact values (e.g., expect(...).toHaveTextContent("6") / "2"); also apply the same change for the other occurrences noted around lines 513-514 to stop relying on section.textContent.Source: Coding guidelines
18-29: ⚡ Quick winAlign test bootstrap with the required
renderPage(type)helper stack.This helper directly uses
render(...)and skips the mandated shared wrapper pattern (MemoryRouter,QueryClient,ThemeProviderviarenderPage(type)).As per coding guidelines, "Use
renderPage(type)helper withMemoryRouter,QueryClient, andThemeProviderin unit tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/editors/__tests__/parser-editor.test.tsx` around lines 18 - 29, The test helper renderEditor currently calls render(...) directly and bypasses the shared wrapper stack; change it to use the shared renderPage(type) helper so tests get MemoryRouter, QueryClient and ThemeProvider applied. Update the renderEditor function (which returns onChange and the render result) to call renderPage(...) with the appropriate page type for this editor (instead of render(...)) and spread/return its result along with onChange so ParserEditor is wrapped consistently.Source: Coding guidelines
src/components/editors/__tests__/parser-workflow-inline.test.tsx (1)
43-131: ⚡ Quick winStrengthen test assertions to ensure parser step coverage.
Tests 2-5 use conditional checks (
if (configButtons.length > 0)) that make them pass even when no parser steps exist in the mock data. This weakens test coverage because the tests won't catch regressions if the inline edit feature breaks but the mock workflow happens to lack parser steps.Consider one of these approaches:
- Assert that at least one parser step exists in the mock data before testing Configure button behavior
- Update the mock data (in
src/test/mocks/handlers.ts) to guarantee a workflow with parser steps for this test suite- Use a dedicated test-only workflow ID that returns parser steps
💚 Example: Assert parser step existence
it("shows Configure button for parser steps (no config URI)", async () => { renderWorkflowPage(); await waitFor(() => { - // The workflow mock data should contain a parser step - // Check that at least one configure-inline button exists const configButtons = screen.queryAllByTestId(/^configure-inline-/); - // Parser step may or may not be in mock workflow data. - // If it exists, verify the button is there - if (configButtons.length > 0) { - expect(configButtons[0]).toBeInTheDocument(); - } + // Assert that parser steps exist in mock data + expect(configButtons.length).toBeGreaterThan(0); + expect(configButtons[0]).toBeInTheDocument(); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/editors/__tests__/parser-workflow-inline.test.tsx` around lines 43 - 131, The test cases ("opens parser dialog when Configure is clicked", "closes parser dialog on cancel", "applies parser changes and marks workflow dirty" and the Configure button presence test) silently skip when no parser step exists because they use if (configButtons.length > 0); fix by adding a hard assertion that a parser configure button exists before proceeding (e.g., expect(configButtons.length).toBeGreaterThan(0)) or ensure the test harness returns a workflow with parser steps (update the mock in handlers.ts or use a dedicated test workflow ID), and then proceed with the existing click and dialog assertions (reference the test names and the queryAllByTestId(/^configure-inline-/) lookup and parser-edit-dialog/parser-editor/assertions).src/pages/__tests__/resource-detail-parser.test.tsx (1)
2-3: 💤 Low valueConsider combining imports from the same module.
Both lines import from
@testing-library/reactand can be combined into a single import statement for better code organization.♻️ Proposed consolidation
-import { screen, waitFor } from "`@testing-library/react`"; -import { render } from "`@testing-library/react`"; +import { render, screen, waitFor } from "`@testing-library/react`";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/__tests__/resource-detail-parser.test.tsx` around lines 2 - 3, The test file imports from `@testing-library/react` in two statements; consolidate them into one import that includes screen, waitFor, and render to clean up redundancy—update the import lines to a single statement importing screen, waitFor, and render (referencing the symbols screen, waitFor, render in resource-detail-parser.test.tsx) so the file uses one organized import from "`@testing-library/react`".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/editors/parser-editor.tsx`:
- Around line 189-194: The new regular-dictionary entry stores the raw input
even though you validate with uri.trim(); update the creation path that
constructs ParserExtensionItem (the anonymous handler that builds newEntry with
type REGULAR_DICT_TYPE and config: { uri }) to normalize the URI before
persisting (e.g., assign a trimmed/normalized value and store that in config.uri
instead of the original input); ensure any subsequent uses of that field read
the normalized value.
- Line 434: The placeholder string for the dictionary URI is hardcoded; replace
it with an i18n call using an inline fallback (e.g.
t("parserEditor.dictionaryUriPlaceholder",
"eddi://ai.labs.dictionary/dictionarystore/dictionaries/<id>?version=<v>")).
Update the JSX placeholder prop in the ParserEditor component (the placeholder
attribute currently set to
"eddi://ai.labs.dictionary/dictionarystore/dictionaries/<id>?version=<v>") to
call t(...), and ensure the component uses the existing translation hook (e.g.,
useTranslation/t) or imports it if missing.
In `@src/i18n/locales/ar.json`:
- Around line 481-484: Replace the English value for the "snippets.description"
key with an Arabic translation so the Arabic locale is fully localized; update
the "snippets" object’s "description" entry
(resources.types.snippets.description / the "snippets": { "description": ... }
field) to a suitable Arabic string such as "مكونات قابلة لإعادة الاستخدام لبناء
موجهات النظام".
In `@src/lib/api/__tests__/extensions.test.ts`:
- Around line 206-207: The test currently excludes the parser URI from
cross-reference integrity checks by adding "eddi://ai.labs.parser" to the SKIP
Set in extensions.test.ts; remove that entry so the parser is no longer skipped
and will be validated by the test (the parser is defined in EXTENSION_TYPE_INFO,
so leaving it out weakens the contract), i.e., edit the SKIP constant to only
include "eddi://ai.labs.snippet" and "eddi://ai.labs.snippets" so
EXTENSION_TYPE_INFO's parser gets checked.
In `@src/pages/workflow-detail.tsx`:
- Around line 684-692: The Escape key handler in the useCallback (handleKeyDown)
only calls e.stopPropagation() and should also call e.preventDefault() to fully
cancel the event; update the handler for the "Escape" branch to call
e.preventDefault() before invoking onCancel() so default browser actions are
suppressed and the event is fully cancelled.
- Line 711: The className string containing "mx-4" is not RTL-aware; update the
JSX element whose className equals "relative z-10 w-full max-w-lg max-h-[85vh]
overflow-y-auto rounded-xl border border-border bg-card shadow-2xl mx-4 p-5
outline-none animate-in fade-in zoom-in-95 duration-200" to use the logical
margin utility instead of the directional one by replacing "mx-4" with
"inline-4" (preserve all other classes and variants).
- Around line 270-304: The handlers handleEditInline and handleParserSave
currently use unsafe type assertions between WorkflowExtension fields and
ParserData (lines around setParserEditData and updated[parserEditIndex]
assignments); add a small runtime validator (e.g., isValidParserData) that
checks required shape/keys of ParserData (and fallback to
createDefaultParserData() or abort/save no-op on failure), call it before
calling setParserEditData in handleEditInline and before applying parserEditData
in handleParserSave, and only cast/assign after validation to prevent corrupting
backend extensions when structure mismatches.
---
Nitpick comments:
In `@src/components/editors/__tests__/parser-editor.test.tsx`:
- Around line 489-495: Replace assertions that inspect section.textContent with
checks against explicit badge-level data-testids: add a stable data-testid on
the badge elements (e.g., "parser-dictionaries-badge" and
"parser-corrections-badge") in the component that renders each section, then
update the test (parser-editor.test.tsx) to use
screen.getByTestId("parser-dictionaries-badge") and
screen.getByTestId("parser-corrections-badge") and assert their exact values
(e.g., expect(...).toHaveTextContent("6") / "2"); also apply the same change for
the other occurrences noted around lines 513-514 to stop relying on
section.textContent.
- Around line 18-29: The test helper renderEditor currently calls render(...)
directly and bypasses the shared wrapper stack; change it to use the shared
renderPage(type) helper so tests get MemoryRouter, QueryClient and ThemeProvider
applied. Update the renderEditor function (which returns onChange and the render
result) to call renderPage(...) with the appropriate page type for this editor
(instead of render(...)) and spread/return its result along with onChange so
ParserEditor is wrapped consistently.
In `@src/components/editors/__tests__/parser-workflow-inline.test.tsx`:
- Around line 43-131: The test cases ("opens parser dialog when Configure is
clicked", "closes parser dialog on cancel", "applies parser changes and marks
workflow dirty" and the Configure button presence test) silently skip when no
parser step exists because they use if (configButtons.length > 0); fix by adding
a hard assertion that a parser configure button exists before proceeding (e.g.,
expect(configButtons.length).toBeGreaterThan(0)) or ensure the test harness
returns a workflow with parser steps (update the mock in handlers.ts or use a
dedicated test workflow ID), and then proceed with the existing click and dialog
assertions (reference the test names and the
queryAllByTestId(/^configure-inline-/) lookup and
parser-edit-dialog/parser-editor/assertions).
In `@src/pages/__tests__/resource-detail-parser.test.tsx`:
- Around line 2-3: The test file imports from `@testing-library/react` in two
statements; consolidate them into one import that includes screen, waitFor, and
render to clean up redundancy—update the import lines to a single statement
importing screen, waitFor, and render (referencing the symbols screen, waitFor,
render in resource-detail-parser.test.tsx) so the file uses one organized import
from "`@testing-library/react`".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24912dcf-ac45-4abf-9301-34fb9bb3a112
📒 Files selected for processing (24)
src/components/editors/__tests__/parser-editor.test.tsxsrc/components/editors/__tests__/parser-workflow-inline.test.tsxsrc/components/editors/editor-registry.tsxsrc/components/editors/parser-editor-types.tssrc/components/editors/parser-editor.tsxsrc/components/editors/pipeline-builder.tsxsrc/i18n/locales/ar.jsonsrc/i18n/locales/de.jsonsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/hi.jsonsrc/i18n/locales/ja.jsonsrc/i18n/locales/ko.jsonsrc/i18n/locales/pt.jsonsrc/i18n/locales/th.jsonsrc/i18n/locales/zh.jsonsrc/lib/api/__tests__/extensions.test.tssrc/lib/api/__tests__/resources.test.tssrc/lib/api/extensions.tssrc/lib/api/resources.tssrc/pages/__tests__/resource-detail-parser.test.tsxsrc/pages/workflow-detail.tsxsrc/test/mocks/handlers.ts
There was a problem hiding this comment.
Pull request overview
This PR adds a dedicated Parser editor UI and wires it into both the resource detail view (standalone parser resources) and workflow pipelines (inline editing for embedded parser steps), along with expanded i18n strings and new tests.
Changes:
- Introduces
ParserEditor+ shared parser types/defaults, and registers the newparsereditor type in the editor registry. - Extends pipeline/workflow UI to support an inline “Configure” action intended for embedded parser steps.
- Adds parser as a first-class resource type (API config + mappings), updates MSW mocks, adds/updates tests, and adds locale strings across multiple languages.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/mocks/handlers.ts | Adds MSW mock response for parser resource data (config + extensions). |
| src/pages/workflow-detail.tsx | Adds parser inline-edit dialog wiring and parser-specific defaults when adding steps. |
| src/pages/tests/resource-detail-parser.test.tsx | New page-level tests asserting parser editor renders in resource detail. |
| src/lib/api/resources.ts | Adds parser to RESOURCE_TYPES for standalone resource support. |
| src/lib/api/extensions.ts | Maps eddi://ai.labs.parser to the parser resource slug. |
| src/lib/api/tests/resources.test.ts | Updates resource type count and asserts parser slug exists. |
| src/lib/api/tests/extensions.test.ts | Updates counts/mappings for parser; validates resource-store mapping assumptions. |
| src/i18n/locales/zh.json | Adds parser resource type labels + parserEditor strings. |
| src/i18n/locales/th.json | Adds parser resource type labels + parserEditor strings. |
| src/i18n/locales/pt.json | Adds parser resource type labels + parserEditor strings. |
| src/i18n/locales/ko.json | Adds parser resource type labels + parserEditor strings. |
| src/i18n/locales/ja.json | Adds parser resource type labels + parserEditor strings. |
| src/i18n/locales/hi.json | Adds parser resource type labels + parserEditor strings. |
| src/i18n/locales/fr.json | Adds parser resource type labels + parserEditor strings. |
| src/i18n/locales/es.json | Adds parser resource type labels + parserEditor strings. |
| src/i18n/locales/en.json | Adds parser resource type labels + parserEditor strings (source language). |
| src/i18n/locales/de.json | Adds parser resource type labels + parserEditor strings. |
| src/i18n/locales/ar.json | Adds parser resource type labels + parserEditor strings. |
| src/components/editors/pipeline-builder.tsx | Adds inline “Configure” button hook intended for embedded parser config. |
| src/components/editors/parser-editor.tsx | New parser form editor UI for parser config/extensions. |
| src/components/editors/parser-editor-types.ts | New shared parser types/constants + createDefaultParserData. |
| src/components/editors/editor-registry.tsx | Registers parser editor type and maps parser extension → slug. |
| src/components/editors/tests/parser-workflow-inline.test.tsx | New workflow-detail tests for parser inline editing (currently conditional). |
| src/components/editors/tests/parser-editor.test.tsx | New unit tests for parser editor behavior and defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix failing studio-editor-panel test: parser now has a registered editor - Normalize URI (trim whitespace) before persisting regular dictionaries - Localize dictionary URI placeholder via t() with new dictUriPlaceholder key - Translate ar.json snippets.description from English to Arabic - Add preventDefault() to Escape key handler in parser dialog - Use logical CSS property mi-4 instead of mx-4 for RTL support - Restrict isInlineEditable to parser steps only (not all steps without URI) - Only inject default parser extensions when no configUri is provided - Remove parser from SKIP set in extension cross-reference test - Add parser to BACKEND_EXTENSIONS in alignment test - Combine duplicate @testing-library/react imports - Propagate dictUriPlaceholder i18n key to all 11 locale files
…tation - Fix mi-4 → mx-4 (mi-4 is not a built-in Tailwind v4 utility) - Add inline parser step (no config.uri) to workflow mock data - Rewrite parser-workflow-inline tests: remove conditional if-guards that made 4/5 tests no-ops; all 6 tests now deterministic - Fix handleAddExtension indentation (4-space → 2-space)
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
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.
This pull request introduces inline editing support for parser steps in workflow pipelines, including a new parser editor, integration with the pipeline UI, and comprehensive test coverage. It also adds internationalization (i18n) strings for the parser editor in English, German, Arabic, and Spanish.
Major features and changes:
Parser Editor Integration
ParserEditorcomponent and associated types inparser-editor-types.ts, including default configuration and constants for parser dictionaries, corrections, and normalizers.editor-registry.tsxto register theparsereditor type, enabling the UI to render parser steps with the new editor. [1] [2] [3]Pipeline Builder Enhancements
pipeline-builder.tsxto support inline editing for parser steps (those without a config URI but with extensions), including a "Configure" button and callback (onEditInline). [1] [2] [3] [4] [5] [6] [7] [8] [9]Testing
parser-workflow-inline.test.tsxto verify parser step rendering, inline editing behavior, dialog interactions, and workflow state management.Internationalization
These changes collectively enable a localized, inline configuration experience for parser steps in workflow pipelines, improving usability and maintainability.
Summary by CodeRabbit
New Features
Localization
Tests