[codex] add import trim options#180
Conversation
|
Warning Review limit reached
More reviews will be available in 20 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds configurable input trimming controls to the amend workflows across the CLI, REST API, MCP, and web UI. The implementation flows from a new core trim utility module through the importer, into the ChangesInput Trimming Feature for Amend
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10ef39ac1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR introduces import-time whitespace trimming controls for amend/import workflows and wires them through core logic, UI, CLI, REST API, and MCP so behavior remains consistent across surfaces.
Changes:
- Added core import trim settings (
trimInput,trimInputFieldsCsv) and applied them during import-to-GenericDataTable conversion. - Threaded trim settings through the app import/export UI (including a settings disclosure) and through CLI/API/MCP schemas + handlers.
- Added parity and end-to-end tests (core, UI unit tests, CLI/API/MCP integration, and Playwright browser coverage) plus documentation updates.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/cross-surface-amend-trim-parity.test.js | Cross-surface parity checks for core/API/MCP/CLI trim behavior |
| README.md | Documented new CLI flags and REST API amend trim fields |
| packages/core/src/tests/core-api/amendFromTextSpecAndData.test.js | Added core tests for trimInput/trimInputFieldsCsv semantics |
| packages/core/src/index.js | Plumbed trim options into amend flow; exported trim helpers |
| packages/core/js/grid/importer.js | Added importer-level trim settings and applied them post-parse |
| packages/core/js/grid/import-trim-settings.js | New shared normalization + trimming implementation |
| packages/core-ui/src/tests/grid/importer.test.js | Verified importer trimming via core importer in UI test suite |
| packages/core-ui/src/tests/app/import-export-workspace.test.js | Ensured workspace threads trim settings into importer |
| packages/core-ui/src/tests/app/import-export-import-control.test.js | Verified import trim UI emits correct settings payload |
| packages/core-ui/js/gui_components/app/import-export-workspace/import-export-workspace-view.js | Wired trim settings callback through workspace view |
| packages/core-ui/js/gui_components/app/import-export-workspace/import-export-workspace-controller.js | Added trim state to workspace controller + update support |
| packages/core-ui/js/gui_components/app/import-export-workspace/create-import-export-workspace-workflow-service.js | Added workflow handler for updating trim settings |
| packages/core-ui/js/gui_components/app/import-export-workspace/create-import-export-workspace-runtime.js | Connected runtime callback to workflow trim updates |
| packages/core-ui/js/gui_components/app/import-export-workspace/create-import-export-file-transfer-service.js | Applied current trim settings to importer before imports |
| packages/core-ui/js/gui_components/app/import-export-trim-state.js | New shared helpers for trim state creation/application |
| packages/core-ui/js/gui_components/app/import-export-toolbar/import-export-toolbar-view.js | Threaded trim settings callback through toolbar view |
| packages/core-ui/js/gui_components/app/import-export-toolbar/import-export-toolbar-controller.js | Added trim state handling to toolbar controller |
| packages/core-ui/js/gui_components/app/import-export-import-control/import-export-import-control-view.js | Added import settings disclosure + trim controls UI |
| packages/core-ui/js/gui_components/app/import-export-import-control/import-export-import-control-controller.js | Added trim state handling to import control controller |
| docs/frontend-component-migration-plan.md | Noted Storybook coverage for new import trim disclosure |
| docs-src/docs/070-interfaces-and-deployment/050-cli-node-and-bun.md | Documented CLI trim flags and usage |
| docs-src/docs/070-interfaces-and-deployment/040-mcp.md | Documented MCP amend trim inputs + example payload update |
| docs-src/docs/070-interfaces-and-deployment/030-rest-api.md | Documented REST amend trim inputs + example payload update |
| apps/web/src/tests/browser/app/functional/import-export/csv-file-upload-trim-input.spec.js | Playwright test covering selected-field trimming on file upload |
| apps/web/src/tests/browser/app/abstractions/components/import-export-workspace.component.js | Added test component helpers to drive trim UI |
| apps/web/src/stories/import-export-toolbar.stories.js | Storybook story + controls documenting trim UI behavior |
| apps/mcp/src/mcp.test.js | MCP schema + behavior tests for trim options |
| apps/mcp/src/index.js | Exposed trim options in MCP tool input schema |
| apps/cli/src/tests/integration.cli-params.test.js | CLI integration test verifying trim affects amend output |
| apps/cli/src/tests/cli-options.test.js | CLI option parser test for new trim flags |
| apps/cli/src/run-cli.js | Passed parsed trim options into core amend call |
| apps/cli/src/cli-options.js | Added yargs options and mapping for trim flags |
| apps/api/src/tests/health/health-endpoints.spec.js | Verified OpenAPI includes trim properties for amend endpoint |
| apps/api/src/tests/generate/v1-generate-amend.spec.js | API test verifying trimInputFieldsCsv behavior |
| apps/api/src/openapi.js | Documented trim inputs in OpenAPI request schema |
| apps/api/src/api-service.js | Threaded trim inputs into amend execution payload |
Greptile SummaryThis PR adds import-time whitespace trimming across all surfaces — core API, REST API, CLI, MCP, and the browser UI — with a new
Confidence Score: 5/5Safe to merge — trim logic is additive, defaults to off on all surfaces, and is well-covered by unit, integration, and browser tests. The change is purely additive: existing behaviour is unchanged when the new parameters are absent or default. Core trim logic is isolated in a single helper, normalization handles all edge cases (non-string input, duplicates, empty entries), and every surface (API, CLI, MCP, UI) is covered by dedicated tests plus a cross-surface parity suite. No data migration, schema change, or breaking API surface is involved. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as ImportControl (View)
participant WC as WorkspaceController
participant FT as FileTransferService
participant IMP as Importer
participant TRIM as import-trim-settings
UI->>WC: "onImportTrimSettingsChange({trimInput, trimInputFieldsEnabled, trimInputFieldsCsv})"
WC->>WC: setImportTrimSettings → updateProps(createImportTrimState)
Note over WC: state.trimInput / trimInputFieldsEnabled / trimInputFieldsCsv updated
UI->>FT: uploadFile / importFromClipboard / importText
FT->>FT: applyImportTrimSettings()
FT->>IMP: "setImportSettings({trimInput, trimInputFieldsCsv})"
IMP->>IMP: store importSettings
FT->>IMP: toGenericDataTable(type, text)
IMP->>TRIM: applyImportTrimSettingsToDataTable(dataTable, importSettings)
TRIM->>TRIM: normalizeImportTrimSettings
TRIM->>TRIM: mutate rows in-place
TRIM-->>IMP: trimmed dataTable
IMP-->>FT: trimmed dataTable
FT->>WC: setGridFromGenericDataTable(trimmed data)
Reviews (3): Last reviewed commit: "harden import trim settings" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await expect(canvas.getByText('Generating export text...')).toBeVisible(); | ||
| }, | ||
| }; | ||
|
|
||
| export const ImportTrimSettings = { | ||
| args: { | ||
| trimInput: true, | ||
| trimInputFieldsEnabled: true, | ||
| trimInputFieldsCsv: 'Name, Email', | ||
| }, | ||
| render: renderImportExportToolbarStory, | ||
| parameters: { | ||
| docs: { | ||
| description: { | ||
| story: | ||
| 'Documents the import-only trim settings behind the import settings disclosure. Toggle the radio groups and edit the CSV textbox to confirm the emitted settings payload shown in the story log.', | ||
| }, | ||
| }, | ||
| }, | ||
| play: async ({ canvasElement }) => { | ||
| const canvas = within(canvasElement); | ||
| await userEvent.click(canvas.getByLabelText('Import settings')); | ||
| const textInput = canvas.getByRole('textbox', { name: /trim input fields csv/i }); | ||
| await userEvent.clear(textInput); | ||
| await userEvent.type(textInput, 'Name, Role'); | ||
| await expect(canvas.getByText(/"trimInputFieldsCsv":"Name, Role"/)).toBeVisible(); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The ImportTrimSettings story description says "Toggle the radio groups and edit the CSV textbox to confirm the emitted settings payload", but the play function only exercises the CSV text input — the radio group interactions are left for reviewers to do manually. Per the AGENTS.md Storybook quality rules, when docs describe behavior modes, each mode should have its own primary story with a play interaction that clearly demonstrates it. As-is, the trim-all (trimInput: on) mode and the per-field (trimInputFieldsEnabled: selected-fields) mode are described in prose but never automated. A reviewer reading the story docs won't have a verified example for either radio path.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const trimInputFieldsText = root.querySelector('[data-role="trim-input-fields-text"]'); | ||
|
|
||
| expect(trimInputFieldsText.disabled).toBe(true); | ||
|
|
||
| trimInputOnRadio.click(); | ||
| trimInputFieldsSelectedRadio.click(); | ||
| trimInputFieldsText.value = 'Name, Role'; | ||
| trimInputFieldsText.dispatchEvent(new dom.window.Event('input', { bubbles: true })); | ||
|
|
There was a problem hiding this comment.
data-role selectors used where role/name queries are available
AGENTS.md says "prefer role/name queries in DOM/component tests" and allows data-* hooks only when role/name queries are not practical. All three elements here are accessible by their ARIA roles and names: the radios have visible <span> text labels and the text input has aria-label="Trim input fields CSV". Using data-role selectors instead couples the test to the internal DOM contract rather than to user-visible semantics. The Storybook play action for the same component already uses getByRole('radio', { name: 'On' }) and getByRole('textbox', { name: /trim input fields csv/i }), so the role/name approach is already established in this PR.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@packages/core-ui/js/gui_components/app/import-export-import-control/import-export-import-control-view.js`:
- Line 69: Remove the inline attribute interpolation of state.trimInputFieldsCsv
from the input template (the input with data-role="trim-input-fields-text") to
avoid HTML attribute injection; instead rely on the existing property assignment
that sets the element's value later (the code that queries the input and sets
.value). Locate the template string that renders <input ...
value="${state.trimInputFieldsCsv || ''}"> and delete the value="..." portion so
the initial DOM is safe, keeping the subsequent logic that assigns the input's
value via JavaScript intact.
In `@packages/core/js/grid/importer.js`:
- Around line 57-63: setImportSettings is clobbering the existing trimInput flag
when callers pass partial settings (e.g., {trimInputFieldsCsv}) because
trimInput is set to settings.trimInput === true unconditionally; change it so
trimInput is only updated when the incoming settings include a trimInput
property (use settings.hasOwnProperty('trimInput') or check for undefined) and
otherwise preserve this.importSettings.trimInput (falling back to false if that
is undefined); update the assignment in setImportSettings to reference
this.importSettings.trimInput and keep trimInputFieldsCsv logic the same.
🪄 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 Plus
Run ID: 6004bf16-ac53-43ab-be4f-d56c5ad09ec0
📒 Files selected for processing (36)
README.mdapps/api/src/api-service.jsapps/api/src/openapi.jsapps/api/src/tests/generate/v1-generate-amend.spec.jsapps/api/src/tests/health/health-endpoints.spec.jsapps/cli/src/cli-options.jsapps/cli/src/run-cli.jsapps/cli/src/tests/cli-options.test.jsapps/cli/src/tests/integration.cli-params.test.jsapps/mcp/src/index.jsapps/mcp/src/mcp.test.jsapps/web/src/stories/import-export-toolbar.stories.jsapps/web/src/tests/browser/app/abstractions/components/import-export-workspace.component.jsapps/web/src/tests/browser/app/functional/import-export/csv-file-upload-trim-input.spec.jsdocs-src/docs/070-interfaces-and-deployment/030-rest-api.mddocs-src/docs/070-interfaces-and-deployment/040-mcp.mddocs-src/docs/070-interfaces-and-deployment/050-cli-node-and-bun.mddocs/frontend-component-migration-plan.mdpackages/core-ui/js/gui_components/app/import-export-import-control/import-export-import-control-controller.jspackages/core-ui/js/gui_components/app/import-export-import-control/import-export-import-control-view.jspackages/core-ui/js/gui_components/app/import-export-toolbar/import-export-toolbar-controller.jspackages/core-ui/js/gui_components/app/import-export-toolbar/import-export-toolbar-view.jspackages/core-ui/js/gui_components/app/import-export-trim-state.jspackages/core-ui/js/gui_components/app/import-export-workspace/create-import-export-file-transfer-service.jspackages/core-ui/js/gui_components/app/import-export-workspace/create-import-export-workspace-runtime.jspackages/core-ui/js/gui_components/app/import-export-workspace/create-import-export-workspace-workflow-service.jspackages/core-ui/js/gui_components/app/import-export-workspace/import-export-workspace-controller.jspackages/core-ui/js/gui_components/app/import-export-workspace/import-export-workspace-view.jspackages/core-ui/src/tests/app/import-export-import-control.test.jspackages/core-ui/src/tests/app/import-export-workspace.test.jspackages/core-ui/src/tests/grid/importer.test.jspackages/core/js/grid/import-trim-settings.jspackages/core/js/grid/importer.jspackages/core/src/index.jspackages/core/src/tests/core-api/amendFromTextSpecAndData.test.jstests/integration/cross-surface-amend-trim-parity.test.js
| </label> | ||
| <label class="import-export-import-control__settings-text"> | ||
| <span>Field names (CSV)</span> | ||
| <input type="text" data-role="trim-input-fields-text" value="${state.trimInputFieldsCsv || ''}" aria-label="Trim input fields CSV"> |
There was a problem hiding this comment.
Avoid HTML attribute injection in initial trim CSV rendering.
Line 69 injects trimInputFieldsCsv into an innerHTML attribute value. If the string contains quotes/markup, it can break attribute context and inject DOM/script on mount. Since Line 146 already sets the input value safely via property assignment, remove inline value interpolation in the template.
Suggested fix
- <input type="text" data-role="trim-input-fields-text" value="${state.trimInputFieldsCsv || ''}" aria-label="Trim input fields CSV">
+ <input type="text" data-role="trim-input-fields-text" aria-label="Trim input fields CSV">🤖 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
`@packages/core-ui/js/gui_components/app/import-export-import-control/import-export-import-control-view.js`
at line 69, Remove the inline attribute interpolation of
state.trimInputFieldsCsv from the input template (the input with
data-role="trim-input-fields-text") to avoid HTML attribute injection; instead
rely on the existing property assignment that sets the element's value later
(the code that queries the input and sets .value). Locate the template string
that renders <input ... value="${state.trimInputFieldsCsv || ''}"> and delete
the value="..." portion so the initial DOM is safe, keeping the subsequent logic
that assigns the input's value via JavaScript intact.
| setImportSettings(settings = {}) { | ||
| this.importSettings = { | ||
| ...this.importSettings, | ||
| ...settings, | ||
| trimInput: settings.trimInput === true, | ||
| trimInputFieldsCsv: settings.trimInputFieldsCsv ?? this.importSettings.trimInputFieldsCsv ?? '', | ||
| }; |
There was a problem hiding this comment.
Preserve previously enabled trimInput on partial settings updates.
setImportSettings({ trimInputFieldsCsv: ... }) currently forces trimInput back to false, so sequential partial updates can silently disable global trimming.
Suggested fix
setImportSettings(settings = {}) {
+ const hasTrimInput = Object.prototype.hasOwnProperty.call(settings, 'trimInput');
this.importSettings = {
...this.importSettings,
...settings,
- trimInput: settings.trimInput === true,
+ trimInput: hasTrimInput ? settings.trimInput === true : this.importSettings.trimInput,
trimInputFieldsCsv: settings.trimInputFieldsCsv ?? this.importSettings.trimInputFieldsCsv ?? '',
};
}📝 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.
| setImportSettings(settings = {}) { | |
| this.importSettings = { | |
| ...this.importSettings, | |
| ...settings, | |
| trimInput: settings.trimInput === true, | |
| trimInputFieldsCsv: settings.trimInputFieldsCsv ?? this.importSettings.trimInputFieldsCsv ?? '', | |
| }; | |
| setImportSettings(settings = {}) { | |
| const hasTrimInput = Object.prototype.hasOwnProperty.call(settings, 'trimInput'); | |
| this.importSettings = { | |
| ...this.importSettings, | |
| ...settings, | |
| trimInput: hasTrimInput ? settings.trimInput === true : this.importSettings.trimInput, | |
| trimInputFieldsCsv: settings.trimInputFieldsCsv ?? this.importSettings.trimInputFieldsCsv ?? '', | |
| }; | |
| } |
🤖 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 `@packages/core/js/grid/importer.js` around lines 57 - 63, setImportSettings is
clobbering the existing trimInput flag when callers pass partial settings (e.g.,
{trimInputFieldsCsv}) because trimInput is set to settings.trimInput === true
unconditionally; change it so trimInput is only updated when the incoming
settings include a trimInput property (use settings.hasOwnProperty('trimInput')
or check for undefined) and otherwise preserve this.importSettings.trimInput
(falling back to false if that is undefined); update the assignment in
setImportSettings to reference this.importSettings.trimInput and keep
trimInputFieldsCsv logic the same.
Summary
Why
This adds a consistent way to trim imported input values globally or by exact header name during amend/import workflows, without expanding the generator UI surface.
Validation
Summary by CodeRabbit
Release Notes
New Features
Documentation