Repository navigation
fix: add disable ttl table action - #3938
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the schema-tree “Alter table” query templates/actions to correctly separate enabling TTL from disabling TTL for row tables, aligning the UI templates with YDB’s actual TTL disable syntax and the linked issue (#3937).
Changes:
- Renamed the previous TTL action/template to an explicit “Enable TTL…” template and removed the incorrect “disable via zero interval” hint.
- Added a dedicated “Disable TTL…” action for row tables that inserts
ALTER TABLE <table> RESET (TTL);with a documentation link. - Added unit tests covering both the enable and disable TTL templates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/containers/Tenant/utils/schemaQueryTemplates.ts | Splits TTL templates into enable vs disable (RESET) variants and removes the incorrect disable hint. |
| src/containers/Tenant/utils/schemaActions.tsx | Wires the new “Enable TTL…” action and limits “Disable TTL…” to row tables as intended. |
| src/containers/Tenant/utils/test/schemaQueryTemplates.test.ts | Adds focused unit tests for the updated TTL templates. |
| src/containers/Tenant/i18n/en.json | Introduces the new “Enable TTL…” label and renames the existing TTL label to “Disable TTL…”. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bc2e0c480
ℹ️ 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".
| const alterColumnTableGroupItem = { | ||
| text: i18n('actions.alterTable'), | ||
| items: [manageColumnsItem, manageAutoPartitioningItem, manageTTLItem], | ||
| items: [manageColumnsItem, manageAutoPartitioningItem, enableTTLItem], |
There was a problem hiding this comment.
Include disable TTL entry in column table actions
The column-table Alter table menu now only exposes enableTTLItem, while the new disable template (manageTTLTemplate) is not reachable there. This means users working with column-oriented tables can no longer disable TTL from the built-in schema actions (the previous manageTTL action was removed from this group), which is a functional regression in the UI workflow for existing TTL-enabled column tables.
Useful? React with 👍 / 👎.
|
@copilot address review comments |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60b21e71d5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const alterColumnTableGroupItem = { | ||
| text: i18n('actions.alterTable'), | ||
| items: [manageColumnsItem, manageAutoPartitioningItem, manageTTLItem], | ||
| items: [manageColumnsItem, manageAutoPartitioningItem, enableTTLItem], |
There was a problem hiding this comment.
Keep disable TTL action for column tables
Removing manageTTLItem from the column-table Alter table menu creates a regression for TTL-enabled column tables: users can still enable TTL via enableTTLItem, but they no longer have a built-in action to generate the corresponding ALTER TABLE ... RESET (TTL) query and must craft it manually. Since this commit introduces a dedicated disable template and row tables still expose it, column tables should expose the same disable action to preserve the previous workflow.
Useful? React with 👍 / 👎.
|
@copilot check naming! for now it's not manage, its disable. Fix all vars and test names. |
Done in f83df1f. All
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f83df1fc33
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const alterColumnTableGroupItem = { | ||
| text: i18n('actions.alterTable'), | ||
| items: [manageColumnsItem, manageAutoPartitioningItem, manageTTLItem], | ||
| items: [manageColumnsItem, manageAutoPartitioningItem, enableTTLItem], |
There was a problem hiding this comment.
Add disable TTL action to column table alter menu
Column-table actions now omit disableTTLItem, so users with TTL-enabled column tables no longer have a built-in way to generate ALTER TABLE ... RESET (TTL) from the UI. This commit also removed the old zero-interval disable hint from the enable template, so the previous in-menu disable path is gone entirely for column tables while row tables still have it, creating a functional regression in the column-table workflow.
Useful? React with 👍 / 👎.
| const alterColumnTableGroupItem = { | ||
| text: i18n('actions.alterTable'), | ||
| items: [manageColumnsItem, manageAutoPartitioningItem, manageTTLItem], | ||
| items: [manageColumnsItem, manageAutoPartitioningItem, enableTTLItem, disableTTLItem], |
There was a problem hiding this comment.
disableTTLItem added to column table menu, contradicting the PR description
The PR description explicitly states "Kept column tables on the enable-TTL template only," but disableTTLItem is included in alterColumnTableGroupItem here. If ALTER TABLE … RESET (TTL) is not supported for column tables in YDB, surfacing this action would let users run a query that will always fail. The row-table group correctly lists both items — the column-table group should likely include only enableTTLItem.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/containers/Tenant/utils/schemaActions.tsx
Line: 370
Comment:
**`disableTTLItem` added to column table menu, contradicting the PR description**
The PR description explicitly states "Kept column tables on the enable-TTL template only," but `disableTTLItem` is included in `alterColumnTableGroupItem` here. If `ALTER TABLE … RESET (TTL)` is not supported for column tables in YDB, surfacing this action would let users run a query that will always fail. The row-table group correctly lists both items — the column-table group should likely include only `enableTTLItem`.
How can I resolve this? If you propose a fix, please make it concise.9b53f85 to
f83df1f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f83df1fc33
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
What changed
Enable TTL...and removed the incorrect zero-interval disable hint from that template.Disable TTL...action that insertsALTER TABLE <table> RESET (TTL);with a docs link.Closes #3937.
Validation
npm run typechecknpm run lint(passes with existing warnings)npm test -- --runInBandCI Results
Test Status:⚠️ FLAKY
📊 Full Report
😟 No changes in tests. 😕
Bundle Size: ✅
Current: 63.96 MB | Main: 63.96 MB
Diff: +1.24 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information