[DO NOT MERGE] Adds transform-index commands for v64 - #44
Conversation
📝 WalkthroughWalkthroughAdds the ChangesTransform index management
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant MetabaseAPI
participant TransformRunner
participant Warehouse
CLI->>MetabaseAPI: Create transform index request
MetabaseAPI-->>CLI: Return create-pending status
CLI->>TransformRunner: Run full transform
TransformRunner->>Warehouse: Apply physical index
Warehouse-->>MetabaseAPI: Report index result
MetabaseAPI-->>CLI: Return succeeded status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
alxnddr
changed the title
![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 (3)
src/domain/transform-index.ts (1)
152-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
.strip()the final compact-schema operation.
TransformIndexCompactextends the schema after stripping it. Move.strip()after.extend()so this follows the required compact-schema contract.Proposed fix
export const TransformIndexCompact = TransformIndex.pick({ name: true, kind: true, key_columns: true, is_unique: true, is_primary: true, metabase_managed: true, present_in_warehouse: true, }) - .strip() - .extend({ request: TransformIndexRequestCompact.optional() }); + .extend({ request: TransformIndexRequestCompact.optional() }) + .strip();As per coding guidelines, “Compact resource schemas must be
<Resource>.pick({...}).strip()” and “the trailing.strip()is mandatory.”🤖 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/domain/transform-index.ts` around lines 152 - 162, Update TransformIndexCompact so TransformIndexRequestCompact is added via .extend() before the final .strip() call. Ensure .strip() is the last schema operation while preserving the selected fields and optional request property.Source: Coding guidelines
src/commands/transform-index/list.ts (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the API response schema into the domain layer.
TransformIndexApiListis a cross-network parsing contract in a command file. Export a named list-response schema fromsrc/domain/transform-index.tsand import it here; retainTransformIndexListEnvelopeas this command’s output schema.As per coding guidelines, “every cross-network value must have a named Zod schema in
src/domain/” and “Commands are CLI shell only: no HTTP, no parsing, and no formatting logic in command files.”🤖 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/commands/transform-index/list.ts` at line 14, Move the named TransformIndexApiList Zod schema from the command file into the transform-index domain module and export it there, then import and reuse it in the command. Keep TransformIndexListEnvelope defined as the command’s output schema and remove the local API response schema declaration.Source: Coding guidelines
tests/e2e/transform-index.e2e.test.ts (1)
106-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the behavior-describing comment.
The helper name and assertions communicate the operation; keep comments only for non-obvious rationale.
As per coding guidelines, “Avoid comments unless the WHY is non-obvious; never write WHAT comments.”
🤖 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 `@tests/e2e/transform-index.e2e.test.ts` around lines 106 - 107, Remove the behavior-describing comment immediately before the table-target transform test; keep the helper invocation and assertions unchanged.Source: Coding guidelines
🤖 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 `@tests/e2e/transform-index.e2e.test.ts`:
- Around line 21-24: Update the transform/index e2e tests around
seedTransformIndex and the affected test cases so each test creates isolated
server-side state: return the newly created transform ID, generate unique
transform target table and index names per test, and pass the returned ID into
all create, list, and run assertions instead of relying on FIRST_TRANSFORM_ID or
fixed names.
---
Nitpick comments:
In `@src/commands/transform-index/list.ts`:
- Line 14: Move the named TransformIndexApiList Zod schema from the command file
into the transform-index domain module and export it there, then import and
reuse it in the command. Keep TransformIndexListEnvelope defined as the
command’s output schema and remove the local API response schema declaration.
In `@src/domain/transform-index.ts`:
- Around line 152-162: Update TransformIndexCompact so
TransformIndexRequestCompact is added via .extend() before the final .strip()
call. Ensure .strip() is the last schema operation while preserving the selected
fields and optional request property.
In `@tests/e2e/transform-index.e2e.test.ts`:
- Around line 106-107: Remove the behavior-describing comment immediately before
the table-target transform test; keep the helper invocation and assertions
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: 09e44315-cd1b-403f-a6e7-35bd83433d9d
📒 Files selected for processing (12)
README.mdpackage.jsonsrc/commands/transform-index/create.tssrc/commands/transform-index/delete.tssrc/commands/transform-index/get.tssrc/commands/transform-index/index.tssrc/commands/transform-index/list.tssrc/commands/transform-index/update.tssrc/domain/transform-index.tssrc/main.tssrc/runtime/command-help.test.tstests/e2e/transform-index.e2e.test.ts
| const FIRST_TRANSFORM_ID = 1; | ||
| const INDEX_NAME = "idx_one"; | ||
| const TARGET_COLUMN = "one"; | ||
| const TRANSFORM_TARGET_TABLE = "e2e_index_transform"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make each test use its own server-side transform and index state.
afterEach only deletes config directories. After the first test, subsequent seedTransformIndex() calls still assert that a newly created transform has ID 1 and create idx_one against transform 1; the suite will fail on the next test due to the incremented transform ID or duplicate index request. Return the created transform ID, generate a unique target table/name per test, and pass that ID through create/list/run assertions.
Also applies to: 62-104
🤖 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 `@tests/e2e/transform-index.e2e.test.ts` around lines 21 - 24, Update the
transform/index e2e tests around seedTransformIndex and the affected test cases
so each test creates isolated server-side state: return the newly created
transform ID, generate unique transform target table and index names per test,
and pass the returned ID into all create, list, and run assertions instead of
relying on FIRST_TRANSFORM_ID or fixed names.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/skill-data/transform/SKILL.md (1)
77-77: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove narrative code-block comments.
These comments describe command behavior rather than a non-obvious rationale. Keep the explanation in surrounding prose and leave the examples uncommented. As per coding guidelines, comments must explain a non-obvious WHY and must not describe WHAT.
Also applies to: 196-196
🤖 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/cli/skill-data/transform/SKILL.md` at line 77, Remove the narrative comments in the Recent runs and corresponding offset-related example sections of SKILL.md, leaving the examples uncommented. Preserve any surrounding prose that explains the command behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@packages/cli/skill-data/transform/SKILL.md`:
- Line 77: Remove the narrative comments in the Recent runs and corresponding
offset-related example sections of SKILL.md, leaving the examples uncommented.
Preserve any surrounding prose that explains the command behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cc38dd1e-fc29-4e41-ab03-f61e35f08bdc
📒 Files selected for processing (10)
README.mdpackages/cli/package.jsonpackages/cli/skill-data/transform/SKILL.mdpackages/cli/src/commands/transform-index/create.tspackages/cli/src/commands/transform-index/delete.tspackages/cli/src/commands/transform-index/get.tspackages/cli/src/commands/transform-index/index.tspackages/cli/src/commands/transform-index/list.tspackages/cli/src/commands/transform-index/update.tspackages/cli/src/main.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
2 participants
Description
Adds commands for the upcoming transform indexes feature. Should be merged after v64 release.
Summary by CodeRabbit
New Features
mb transform-indexcommands to list, view, create, update, and delete managed indexes for transform output tables.--yesfor non-interactive deletion.Documentation
Tests