42 amend calls in api and mcp and cli#78
Closed
eviltester wants to merge 9 commits into
Closed
Conversation
|
👀 QA.tech will run a exploratory tests to review this PR as soon as a deployment is available for this PR. Alternatively, you can comment @qa.tech to manually trigger a review. Learn more about configuring preview deployments. What happens next
🤖 AI end-to-end testing powered by |
|
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 top‑N visible/active column text helpers and a grid-settle poller to GridRendererComponent, wires renderer into GridHeader/GridEditor (extending clearFilters), updates Playwright tests to assert top‑N active values, and publishes amend docs plus a blog post for REST/CLI/MCP. ChangesGrid Renderer Top Helpers & Test Updates
Amend Feature Documentation Across Interfaces
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 1
🧹 Nitpick comments (2)
docs-src/docs/070-interfaces-and-deployment/050-cli-node-and-bun.md (1)
71-72: ⚡ Quick winConsider rewording for better readability.
The documentation is clear and accurate, but the repetitive sentence structure could be improved for better flow.
✨ Suggested rewording to reduce repetition
-- For `amend`, if `-n/--numberOfLines` is omitted, all imported rows are amended. -- For `amend`, if `-n/--numberOfLines` is smaller than imported rows, only the first `N` rows are amended and full output is still returned. +- For `amend`, `-n/--numberOfLines` defaults to all imported rows when omitted. If provided and smaller than the imported count, only the first `N` rows are amended while the full output is still returned.As per coding guidelines: Static analysis tool LanguageTool flagged repetitive sentence beginnings with "For".
🤖 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 `@docs-src/docs/070-interfaces-and-deployment/050-cli-node-and-bun.md` around lines 71 - 72, The two bullets for the `amend` flag start repetitively with "For `amend`"; reword them to improve flow by combining related behavior and varying sentence starts: e.g., state default behavior when `-n/--numberOfLines` is omitted (all imported rows are amended) and then describe the truncated behavior when `-n/--numberOfLines` is smaller than the number of imported rows (only the first N rows are amended but full output is returned), making sure to reference `amend` and `-n/--numberOfLines` in the same concise sentences to avoid repetition.docs-src/blog/2026-05-10-import-and-amend-across-interfaces.md (1)
37-41: ⚡ Quick winConsider rewording for variety.
The content is accurate and valuable, but the repetitive sentence structure could be improved for better readability.
✨ Suggested rewording to reduce repetition
## Why This Matters -- You can automate the same amend workflow previously available in UI. -- You can update existing datasets without rebuilding them from scratch. -- You can keep the same schema-driven logic across UI, REST, CLI, and MCP. +- Automates the same amend workflow previously available in UI. +- Updates existing datasets without rebuilding them from scratch. +- Keeps the same schema-driven logic across UI, REST, CLI, and MCP.As per coding guidelines: Static analysis tool LanguageTool flagged repetitive sentence beginnings with "You can".
🤖 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 `@docs-src/blog/2026-05-10-import-and-amend-across-interfaces.md` around lines 37 - 41, The three bullets under the "## Why This Matters" section use repetitive sentence starts ("You can"); edit the three list items to vary phrasing and improve flow by using different sentence structures and subjects (e.g., change one to an imperative like "Automate the amend workflow previously available in the UI", another to passive/feature-focused like "Existing datasets can be updated without rebuilding from scratch", and the third to emphasize continuity like "Maintain the same schema-driven logic across UI, REST, CLI, and MCP"); update the bullet text directly in that section to preserve meaning while eliminating the repeated "You can" lead-in.
🤖 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
`@apps/web/src/tests/browser/abstractions/components/grid-renderer.component.js`:
- Around line 73-85: The current getTopVisibleColumnTextsByName uses raw row
indexes and can return texts from hidden rows; change it to collect the first
desiredCount visible rows by scanning row indices and checking visibility (use
existing helpers like countVisibleRows and any row-visibility predicate such as
isRowVisible/getRowVisibility or a getRowElement check) and only call
getCellTextByColumnName(columnName, visibleRowIndex) for rows confirmed visible;
stop when you’ve gathered desiredCount or reached the total row count to avoid
infinite loops.
---
Nitpick comments:
In `@docs-src/blog/2026-05-10-import-and-amend-across-interfaces.md`:
- Around line 37-41: The three bullets under the "## Why This Matters" section
use repetitive sentence starts ("You can"); edit the three list items to vary
phrasing and improve flow by using different sentence structures and subjects
(e.g., change one to an imperative like "Automate the amend workflow previously
available in the UI", another to passive/feature-focused like "Existing datasets
can be updated without rebuilding from scratch", and the third to emphasize
continuity like "Maintain the same schema-driven logic across UI, REST, CLI, and
MCP"); update the bullet text directly in that section to preserve meaning while
eliminating the repeated "You can" lead-in.
In `@docs-src/docs/070-interfaces-and-deployment/050-cli-node-and-bun.md`:
- Around line 71-72: The two bullets for the `amend` flag start repetitively
with "For `amend`"; reword them to improve flow by combining related behavior
and varying sentence starts: e.g., state default behavior when
`-n/--numberOfLines` is omitted (all imported rows are amended) and then
describe the truncated behavior when `-n/--numberOfLines` is smaller than the
number of imported rows (only the first N rows are amended but full output is
returned), making sure to reference `amend` and `-n/--numberOfLines` in the same
concise sentences to avoid repetition.
🪄 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: c225f3e2-f6e0-4e5d-b770-dd3f584037f0
📒 Files selected for processing (9)
apps/web/src/tests/browser/abstraction-smoke-tests/grid-header-controls.spec.jsapps/web/src/tests/browser/abstractions/components/grid-renderer.component.jsapps/web/src/tests/browser/app-coverage/filtering-sorting/filtering-sorting.spec.jsapps/web/src/tests/browser/planned-functional/filtering-sorting/column-sorting.spec.jsapps/web/src/tests/browser/planned-functional/grid-operations/clear-sort-before-add-rows-below.spec.jsdocs-src/blog/2026-05-10-import-and-amend-across-interfaces.mddocs-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.md
![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: fa579bc9e7
ℹ️ 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.
♻️ Duplicate comments (1)
apps/web/src/tests/browser/abstractions/components/grid-renderer.component.js (1)
73-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVisible-row helper still indexes raw rows
Line 83 and Line 84 still use raw indices, so hidden rows can be read even when
countVisibleRows()is sufficient. That breaks the “top visible rows” contract and can make assertions flaky.Suggested fix
async getTopVisibleColumnTextsByName(columnName, count) { - const desiredCount = Number(count) || 0; - if (desiredCount <= 0) { + const desiredCount = Math.trunc(Number(count)); + if (!Number.isFinite(desiredCount) || desiredCount <= 0) { return []; } - const visibleRows = await this.countVisibleRows(); - if (visibleRows < desiredCount) { - return []; - } - const values = []; - for (let rowIndex = 0; rowIndex < desiredCount; rowIndex += 1) { - values.push(await this.getCellTextByColumnName(columnName, rowIndex)); - } - return values; + const columnIndex = await this._columnIndexByName(columnName); + return this.rows.evaluateAll((rowEls, { desiredCount, columnIndex }) => { + const visibleRows = rowEls.filter((row) => row.offsetParent !== null).slice(0, desiredCount); + if (visibleRows.length < desiredCount) return []; + return visibleRows.map((row) => { + const cell = row.querySelectorAll('.tabulator-cell')[columnIndex]; + return (cell?.innerText || '').trim(); + }); + }, { desiredCount, columnIndex }); }🤖 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/tests/browser/abstractions/components/grid-renderer.component.js` around lines 73 - 85, getTopVisibleColumnTextsByName currently uses raw row indices (rowIndex) to call getCellTextByColumnName, so hidden rows can be read; change the loop to map the desired visible-row ordinal to the actual DOM row index before calling getCellTextByColumnName. In practice, for each visible ordinal (0..desiredCount-1) obtain the corresponding actual row index (e.g. via an existing helper like countVisibleRows/getVisibleRowIndex or by querying visible row elements and using their index) and pass that actual index into getCellTextByColumnName(columnName, actualRowIndex) so only visible rows are read.
🤖 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.
Duplicate comments:
In
`@apps/web/src/tests/browser/abstractions/components/grid-renderer.component.js`:
- Around line 73-85: getTopVisibleColumnTextsByName currently uses raw row
indices (rowIndex) to call getCellTextByColumnName, so hidden rows can be read;
change the loop to map the desired visible-row ordinal to the actual DOM row
index before calling getCellTextByColumnName. In practice, for each visible
ordinal (0..desiredCount-1) obtain the corresponding actual row index (e.g. via
an existing helper like countVisibleRows/getVisibleRowIndex or by querying
visible row elements and using their index) and pass that actual index into
getCellTextByColumnName(columnName, actualRowIndex) so only visible rows are
read.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ae62e2d-cd5c-417f-828e-e7a6b0f4a3d8
📒 Files selected for processing (5)
apps/web/src/tests/browser/abstraction-smoke-tests/grid-header-controls.spec.jsapps/web/src/tests/browser/abstractions/components/grid-renderer.component.jsapps/web/src/tests/browser/app-coverage/filtering-sorting/filtering-sorting.spec.jsapps/web/src/tests/browser/planned-functional/filtering-sorting/column-sorting.spec.jsapps/web/src/tests/browser/planned-functional/grid-operations/clear-sort-before-add-rows-below.spec.js
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/src/tests/browser/app-coverage/filtering-sorting/filtering-sorting.spec.js
- apps/web/src/tests/browser/planned-functional/grid-operations/clear-sort-before-add-rows-below.spec.js
- apps/web/src/tests/browser/abstraction-smoke-tests/grid-header-controls.spec.js
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 `@apps/web/src/tests/browser/abstractions/components/grid-editor.component.js`:
- Around line 102-104: The recovery check for default mode uses a strict
greater-than which can false-fail when filters clear but the row count stays the
same; in the calculation that sets rowCountRecovered (and the similar check
around lines 116-118) update the condition from activeRowCount >
initialActiveRowCount to activeRowCount >= initialActiveRowCount so that
unchanged-but-cleared results are treated as recovered; locate the expression
using expectedActiveRowCount, activeRowCount, and initialActiveRowCount and
change the operator accordingly.
- Around line 107-108: The call to await this.renderer.waitForGridSettle({
columnName: diagnosticColumn, stableForMs: 2000, timeoutMs: 7000 }) can throw
and currently escapes the surrounding retry loop and final diagnostic/error
block; wrap that await in a try/catch inside the retry loop (in the same
function where retries are performed) so timeout errors are caught,
logged/ignored for the current attempt, and the loop continues to the next
retry, and only after all retries are exhausted rethrow or let the existing
final error handling run (detect the timeout by checking the thrown
error/message and avoid swallowing non-timeout unexpected errors). Ensure you
reference the existing retry loop and the final diagnostic/error block so the
catch does not bypass those.
🪄 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: 370418be-493b-46cf-8039-da4ef5948423
📒 Files selected for processing (5)
apps/web/src/tests/browser/abstractions/components/grid-editor.component.jsapps/web/src/tests/browser/abstractions/components/grid-header.component.jsapps/web/src/tests/browser/abstractions/components/grid-renderer.component.jsapps/web/src/tests/browser/app-coverage/filtering-sorting/filtering-sorting.spec.jsapps/web/src/tests/browser/app-coverage/helpers/test-helpers.js
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.
Summary by CodeRabbit