feat(datasources): add json-layouts and delete commands (FT-1918)#33
Merged
Conversation
WalkthroughThis PR extends the datasources command to support deletion and JSON layout management. Four new HTTP client methods are added for datasource deletion via Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/api-client/api-client.ts (1)
2447-2453: ⚡ Quick winValidate
ok:falseenvelopes for json-layouts mutations.
createDatasourceJsonLayoutsandrecreateDatasourceJsonLayoutscurrently ignore API envelope failures, so a 200 response withok: falsecould be reported as success.Proposed patch
async createDatasourceJsonLayouts(id: DatasourceId): Promise<void> { - await this.request('POST', `/datasources/${id}/json_layouts/create`); + const response = await this.request('POST', `/datasources/${id}/json_layouts/create`); + this.validateOkResponse(response, 'createDatasourceJsonLayouts'); } async recreateDatasourceJsonLayouts(id: DatasourceId): Promise<void> { - await this.request('POST', `/datasources/${id}/json_layouts/recreate`); + const response = await this.request('POST', `/datasources/${id}/json_layouts/recreate`); + this.validateOkResponse(response, 'recreateDatasourceJsonLayouts'); }🤖 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/api-client/api-client.ts` around lines 2447 - 2453, The two methods createDatasourceJsonLayouts and recreateDatasourceJsonLayouts currently call this.request and ignore the API envelope, so they treat a 200 with {ok:false} as success; change each to capture the response (e.g. const res = await this.request(...)) and validate res.ok === true, throwing an Error (including res.error or res.message if present) when ok is false so failures are surfaced instead of swallowed.src/core/datasources/datasources.test.ts (1)
20-37: ⚡ Quick winReset shared mocks between tests for isolation.
mockClientis reused across all specs without a reset hook, so call history/implementations can bleed between cases.Proposed patch
import { describe, it, expect, vi } from 'vitest'; +import { beforeEach } from 'vitest'; @@ describe('datasources', () => { const mockClient = { @@ }; + + beforeEach(() => { + vi.clearAllMocks(); + });Also applies to: 132-165
🤖 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/core/datasources/datasources.test.ts` around lines 20 - 37, The shared mockClient in the describe('datasources', ...) block is reused across tests so call histories and implementations can leak; add a test hook to reset mocks between specs—for example add beforeEach(() => Object.values(mockClient).forEach(fn => fn.mockReset())) or use Vitest's vi.resetAllMocks()/vi.clearAllMocks() in beforeEach/afterEach—apply the same change for the other mock block around lines with the second mockClient (the 132-165 region) so all mock functions (e.g., listDatasources, getDatasource, createDatasource, etc.) are reset before each test.src/commands/datasources/index.ts (1)
209-216: ⚡ Quick winPrefer throwing over direct
process.exit(1)in the recreate guard.The current code's
process.exit(1)bypasseswithErrorHandling's error handler entirely. Throwing allowshandleCommandErrorto manage the exit consistently, consolidating error logging to a single code path rather than having duplicated error output logic.Proposed patch
withErrorHandling(async (id: DatasourceId, options: { yes: boolean }) => { if (!options.yes) { - console.error( - chalk.red( - `✗ Refusing to recreate the json_layouts table on datasource ${id} without --yes. This drops the existing table.` - ) - ); - process.exit(1); + throw new Error( + `Refusing to recreate the json_layouts table on datasource ${id} without --yes. This drops the existing table.` + ); } const globalOptions = getGlobalOptions(jsonLayoutsRecreateCommand);🤖 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/datasources/index.ts` around lines 209 - 216, The recreate guard currently calls process.exit(1) when options.yes is false which bypasses the withErrorHandling/handleCommandError path; instead, throw a descriptive Error (e.g., new Error with the same message) from the block that checks options.yes so the error bubbles up to withErrorHandling/handleCommandError for consistent logging and exit handling; update the block surrounding the options.yes check (the recreate logic in the command handler) to remove process.exit(1) and throw the Error, leaving the existing chalk/red message text intact for the thrown error.
🤖 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.
Nitpick comments:
In `@src/api-client/api-client.ts`:
- Around line 2447-2453: The two methods createDatasourceJsonLayouts and
recreateDatasourceJsonLayouts currently call this.request and ignore the API
envelope, so they treat a 200 with {ok:false} as success; change each to capture
the response (e.g. const res = await this.request(...)) and validate res.ok ===
true, throwing an Error (including res.error or res.message if present) when ok
is false so failures are surfaced instead of swallowed.
In `@src/commands/datasources/index.ts`:
- Around line 209-216: The recreate guard currently calls process.exit(1) when
options.yes is false which bypasses the withErrorHandling/handleCommandError
path; instead, throw a descriptive Error (e.g., new Error with the same message)
from the block that checks options.yes so the error bubbles up to
withErrorHandling/handleCommandError for consistent logging and exit handling;
update the block surrounding the options.yes check (the recreate logic in the
command handler) to remove process.exit(1) and throw the Error, leaving the
existing chalk/red message text intact for the thrown error.
In `@src/core/datasources/datasources.test.ts`:
- Around line 20-37: The shared mockClient in the describe('datasources', ...)
block is reused across tests so call histories and implementations can leak; add
a test hook to reset mocks between specs—for example add beforeEach(() =>
Object.values(mockClient).forEach(fn => fn.mockReset())) or use Vitest's
vi.resetAllMocks()/vi.clearAllMocks() in beforeEach/afterEach—apply the same
change for the other mock block around lines with the second mockClient (the
132-165 region) so all mock functions (e.g., listDatasources, getDatasource,
createDatasource, etc.) are reset before each test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e65ad684-075c-4fb7-bf71-7f7d89772a97
📒 Files selected for processing (5)
src/api-client/api-client.tssrc/commands/datasources/datasources.test.tssrc/commands/datasources/index.tssrc/core/datasources/datasources.test.tssrc/core/datasources/datasources.ts
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
1 participant
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
Wraps four datasource backend endpoints that the CLI was missing:
absmartly datasources delete <id>→DELETE /v1/datasources/:id. Backend rejects with a clear error if the datasource is the default or used by a goal — those propagate verbatim.absmartly datasources json-layouts create <id>→ creates thejson_layoutstable on the datasource.absmartly datasources json-layouts recreate <id> --yes→ drops and recreates the table. Destructive — bails out before any client setup unless--yesis passed.absmartly datasources json-layouts preview <id>→ returns row count + a 5-row sample of thejson_layoutstable.The three
json_layoutsendpoints were added to the office backend on the FT-1865 branch; this PR adds the corresponding CLI surface. Thedeletecommand is for a pre-existing backend endpoint that the CLI had never wrapped.Each command follows the existing three-layer pattern:
APIClientmethod →core/datasourceswrapper → commander subcommand, with tests at each layer that the layer cares about.JIRA: FT-1918
Test plan
bun run test— full suite (2320 tests, all pass)bun run lintbun run buildabsmartly datasources --helplistsdeleteandjson-layoutsabsmartly datasources json-layouts --helplistscreate,recreate,previewabsmartly datasources json-layouts recreate --helpdocuments the--yesflagdeletea non-default unused datasource succeeds;deleteon the default returns the backend's conflict messagejson-layouts create/preview/recreate --yeson a real datasourcejson-layouts recreatewithout--yesprints the red refusal to stderr and exits 1Summary by CodeRabbit
New Features
Tests