configurable row count#150
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR extracts the preview row limit into a dedicated spinbutton control, injects it into TextPreviewEditor via a services parameter, syncs it with legacy ImportExportControls through workspace wiring, and updates stories, tests, styles, and docs to use the new UI and role-based selectors. ChangesPreview Row Limit Control Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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
apps/web/src/stories/export-format-interactions.jsOops! Something went wrong! :( ESLint: 10.2.1 A config object is using the "root" key, which is not supported in flat config system. Flat configs always act as if they are the root config file, so this key can be safely removed. apps/web/src/stories/import-export-workspace.stories.jsOops! Something went wrong! :( ESLint: 10.2.1 A config object is using the "root" key, which is not supported in flat config system. Flat configs always act as if they are the root config file, so this key can be safely removed. apps/web/src/stories/text-preview-editor.stories.jsOops! Something went wrong! :( ESLint: 10.2.1 A config object is using the "root" key, which is not supported in flat config system. Flat configs always act as if they are the root config file, so this key can be safely removed.
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: 5bc0ca5185
ℹ️ 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
Adds a configurable “preview row count” control to the import/export text preview experience so the preview sample size is no longer fixed at 10 rows (closes #140).
Changes:
- Introduces a compact row-count input (min/max constrained) alongside the Preview/Edit toggle and wires it through the app-side workspace into legacy
ImportExportControls. - Updates preview/edit help text and Storybook interaction harnesses to use the new row-count input (instead of embedding the count in the Preview button label).
- Adds/updates Jest tests to cover runtime changes to the preview row limit and basic accessibility (aria-label) for the new control.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core-ui/src/tests/utils/import-export-controls-mode.test.js | Adds coverage that ImportExportControls respects a runtime-updated preview row limit. |
| packages/core-ui/src/tests/app/text-preview-editor.test.js | Verifies the preview row-count control renders, updates help text, and disables in edit mode. |
| packages/core-ui/src/tests/app/import-export-workspace.test.js | Verifies workspace wires the row-count input into state + legacy controls. |
| packages/core-ui/js/gui_components/shared/row-count-control/row-count-control-view.js | Adds optional aria-label handling on the shared numeric input for accessibility. |
| packages/core-ui/js/gui_components/shared/row-count-control/row-count-control-controller.js | Extends row-count control props normalization to include inputAriaLabel. |
| packages/core-ui/js/gui_components/app/text-preview-editor/text-preview-editor-view.js | Renders and updates the row-count control in the preview editor UI; updates help text accordingly. |
| packages/core-ui/js/gui_components/app/text-preview-editor/text-preview-editor-controller.js | Adds a controller API to update previewRowLimit at runtime. |
| packages/core-ui/js/gui_components/app/text-preview-editor/index.js | Wires in the shared row-count control via services injection (defaulting to shared implementation). |
| packages/core-ui/js/gui_components/app/import-export-workspace/index.js | Propagates preview row-limit changes from the preview editor into legacy controls and workspace state. |
| packages/core-ui/js/gui_components/app/import-export-workspace/import-export-workspace-view.js | Passes the new preview row-limit change callback through to the preview editor component. |
| packages/core-ui/js/gui_components/app/import-export-controls.js | Adds setPreviewRowLimit with clamping/normalization for legacy preview-limited rendering paths. |
| apps/web/styles.css | Styles the preview row-count input more compactly for the app preview editor context. |
| apps/web/src/stories/text-preview-editor.stories.js | Updates Storybook assertions for the Preview button and new row-count spinbutton. |
| apps/web/src/stories/import-export-workspace.stories.js | Adds Storybook control for initial previewRowLimit and asserts row-count input presence/value. |
| apps/web/src/stories/export-preview-story-harness.js | Updates story harness to set preview row limit via the input instead of mutating button text/state. |
| apps/web/src/stories/export-format-interactions.js | Updates interaction flows to click “Preview” (no embedded count) and assert row-count control. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/stories/text-preview-editor.stories.js (1)
43-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStory description is stale. The PR moves the row count out of the toggle button into a dedicated spinbutton, so this description no longer matches what the story demonstrates.
📝 Suggested wording
- story: - 'Preview-mode state with Auto Preview enabled and the preview-row count reflected in the toggle button label.', + story: + 'Preview-mode state with Auto Preview enabled and the preview-row count shown in the dedicated "Preview row count" spinbutton (default 10).',Based on learnings: "Add story descriptions to Storybook stories that explain what the story demonstrates, which defaults or limits matter, what the reviewer should try in the UI, and what outcome they should expect."
🤖 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 `@apps/web/src/stories/text-preview-editor.stories.js` around lines 43 - 48, Update the story description string (docs.description.story) in text-preview-editor.stories.js to reflect that the row count is no longer part of the toggle button but is now controlled by a dedicated spinbutton; change the text to briefly state what the story demonstrates (preview mode with Auto Preview enabled, the presence and purpose of the spinbutton controlling preview-row count), note any defaults/limits the spinbutton enforces, and suggest what the reviewer should try in the UI and the expected outcome (e.g., toggling Auto Preview and adjusting the spinbutton updates the preview row count).
🧹 Nitpick comments (1)
apps/web/src/stories/export-format-interactions.js (1)
51-51: ⚡ Quick winNo ambiguity risk from the new spinbutton’s label text for
getByText('Preview')—TextPreviewEditorViewpasseslabel: ''intocreateRowCountControl, andRowCountControlViewrenders that as an empty<span data-role="label">, while the spinbutton’s “Preview row count” text exists only as the input’saria-label. So the only visible “Preview” text should be the mode button, keepinggetByText('Preview')unambiguous (e.g., inplayCsvRoundTripandplayPreviewEditMode). Consider usinggetByRole('button', { name: 'Preview' })for consistency with the existing role-based queries.🤖 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 `@apps/web/src/stories/export-format-interactions.js` at line 51, The test uses getByText('Preview') which is safe here but inconsistent and slightly fragile because RowCountControlView renders an empty label span while the spinbutton's visible label is only aria-label; update the test to query the preview mode button by role instead of text. Replace occurrences of getByText('Preview') in the playCsvRoundTrip and playPreviewEditMode flows with getByRole('button', { name: 'Preview' }), and ensure any helper or wrapper utilities that reference getByText for this control are updated; verify TextPreviewEditorView/createRowCountControl/RowCountControlView behavior remains unchanged.
🤖 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/src/tests/app/text-preview-editor.test.js`:
- Line 49: The test is using previewButton.innerText which is undefined under
jsdom; update the assertions in the test file to read previewButton.textContent
(and call .trim() if whitespace-sensitive) instead of previewButton.innerText
for both occurrences (the checks comparing to 'Preview' and the other
assertion), ensuring the test uses textContent.trim() where needed to match the
expected string.
---
Outside diff comments:
In `@apps/web/src/stories/text-preview-editor.stories.js`:
- Around line 43-48: Update the story description string
(docs.description.story) in text-preview-editor.stories.js to reflect that the
row count is no longer part of the toggle button but is now controlled by a
dedicated spinbutton; change the text to briefly state what the story
demonstrates (preview mode with Auto Preview enabled, the presence and purpose
of the spinbutton controlling preview-row count), note any defaults/limits the
spinbutton enforces, and suggest what the reviewer should try in the UI and the
expected outcome (e.g., toggling Auto Preview and adjusting the spinbutton
updates the preview row count).
---
Nitpick comments:
In `@apps/web/src/stories/export-format-interactions.js`:
- Line 51: The test uses getByText('Preview') which is safe here but
inconsistent and slightly fragile because RowCountControlView renders an empty
label span while the spinbutton's visible label is only aria-label; update the
test to query the preview mode button by role instead of text. Replace
occurrences of getByText('Preview') in the playCsvRoundTrip and
playPreviewEditMode flows with getByRole('button', { name: 'Preview' }), and
ensure any helper or wrapper utilities that reference getByText for this control
are updated; verify
TextPreviewEditorView/createRowCountControl/RowCountControlView behavior remains
unchanged.
🪄 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: 825d6e90-64fa-483d-a4c4-187555e7775b
📒 Files selected for processing (16)
apps/web/src/stories/export-format-interactions.jsapps/web/src/stories/export-preview-story-harness.jsapps/web/src/stories/import-export-workspace.stories.jsapps/web/src/stories/text-preview-editor.stories.jsapps/web/styles.csspackages/core-ui/js/gui_components/app/import-export-controls.jspackages/core-ui/js/gui_components/app/import-export-workspace/import-export-workspace-view.jspackages/core-ui/js/gui_components/app/import-export-workspace/index.jspackages/core-ui/js/gui_components/app/text-preview-editor/index.jspackages/core-ui/js/gui_components/app/text-preview-editor/text-preview-editor-controller.jspackages/core-ui/js/gui_components/app/text-preview-editor/text-preview-editor-view.jspackages/core-ui/js/gui_components/shared/row-count-control/row-count-control-controller.jspackages/core-ui/js/gui_components/shared/row-count-control/row-count-control-view.jspackages/core-ui/src/tests/app/import-export-workspace.test.jspackages/core-ui/src/tests/app/text-preview-editor.test.jspackages/core-ui/src/tests/utils/import-export-controls-mode.test.js
eviltester
deleted the
140-text-to-control-auto-preview-number-rather-than-fix-10-as-default
branch
closes #140
Summary by CodeRabbit
New Features
UI Updates
Accessibility
Tests
Documentation