fix(platform-config): send full config payload on update (FT-1941)#38
fix(platform-config): send full config payload on update (FT-1941)#38joalves wants to merge 8 commits into
Conversation
…FT-1941) Polish from final review of FT-1941: a one-line comment on the destructure explaining why id is dropped (it lives in the URL path), and an end-to-end CLI test that exercises a numeric --value to lock in the cast removal.
WalkthroughThis pull request changes platform config updates to read-before-write: updatePlatformConfig now GETs the existing config, ensures it's a non-null object, removes its id, merges the provided value into the remaining fields, and PUTs the merged record (excluding id). UpdatePlatformConfigParams.value is relaxed to unknown. Tests in core, command, and API layers were added or adjusted to verify merging, diverse JSON value handling, error cases, and the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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
src/core/platformconfig/platformconfig.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/core/platformconfig/platformconfig.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Comment |
…FT-1941) The pre-PUT GET in updatePlatformConfig can fail two ways the previous code mishandled: - GET throws (404, network, auth): the error reached the user without context, looking like a fetch error from an update command. - GET returns 200 with a non-object payload: surfaced as "existing config not found", which actively misled debugging — the real fetch path throws on 404, so this branch only fires for malformed responses. Wrap the GET in try/catch and rethrow with cause, and reword the non-object branch to name the actual condition. Also document the last-writer-wins merge semantics inline.
- Rename the not-found test to assert the new malformed-payload wording and verify no PUT is attempted. - Add a test for the GET-rejects path, asserting wrap message and that PUT is not attempted. - Parametrize over falsy JSON values (null, false, 0, "", []) to pin the value:unknown contract — a future truthiness guard would regress these without the test.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/platformconfig/platformconfig.test.ts (1)
64-83: ⚡ Quick winConsider adding a
beforeEachhook for consistent mock cleanup.The selective
mockClear()calls on lines 66 and 77 are correct (only negative-assertion tests need them), but adding abeforeEachhook to clear all mocks would improve maintainability and prevent future test isolation issues.♻️ Proposed refactor to add beforeEach hook
Add this after line 9:
mockClient = { listPlatformConfigs: vi.fn(), getPlatformConfig: vi.fn(), updatePlatformConfig: vi.fn(), }; + + beforeEach(() => { + vi.clearAllMocks(); + });Then remove the manual
mockClear()calls on lines 66 and 77:it('should throw a malformed-payload error if GET returns a non-object', async () => { mockClient.getPlatformConfig.mockResolvedValue(null); - mockClient.updatePlatformConfig.mockClear();it('should wrap GET errors with context and not attempt a PUT', async () => { const cause = new Error('network down'); mockClient.getPlatformConfig.mockRejectedValue(cause); - mockClient.updatePlatformConfig.mockClear();🤖 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/platformconfig/platformconfig.test.ts` around lines 64 - 83, Add a beforeEach hook in the platformconfig.test.ts to call mockClient.getPlatformConfig.mockClear() and mockClient.updatePlatformConfig.mockClear() (or jest.clearAllMocks()) so each test starts with clean mock state, then remove the manual mockClear() invocations currently inside the two tests that assert negative behavior (references: mockClient.getPlatformConfig, mockClient.updatePlatformConfig, and tests invoking updatePlatformConfig).
🤖 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/core/platformconfig/platformconfig.test.ts`:
- Around line 64-83: Add a beforeEach hook in the platformconfig.test.ts to call
mockClient.getPlatformConfig.mockClear() and
mockClient.updatePlatformConfig.mockClear() (or jest.clearAllMocks()) so each
test starts with clean mock state, then remove the manual mockClear()
invocations currently inside the two tests that assert negative behavior
(references: mockClient.getPlatformConfig, mockClient.updatePlatformConfig, and
tests invoking updatePlatformConfig).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fc30a5f-3e35-47f1-b369-b92064d1ee66
📒 Files selected for processing (2)
src/core/platformconfig/platformconfig.test.tssrc/core/platformconfig/platformconfig.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/platformconfig/platformconfig.ts
1 participant
Summary
Fixes FT-1941.
abs platform-config update <id> --value <json>was returning HTTP 500 (Cannot use 'in' operator to search for 'archived' in 30) because the CLI was sending{"data": <raw-value>}instead of the required{"data": {"name": "...", "value": "...", ...}}.src/core/platformconfig/platformconfig.ts): nowGETs the current config, merges the newvalueover it, dropsid(it's in the URL path), andPUTs the full object.UpdatePlatformConfigParams.valueis nowunknownso the CLI can pass any JSON value (string, number, object, array).src/commands/platformconfig/index.ts): dropped the misleadingas Record<string, unknown>cast onvalidateJSON(--value). Configvalues in the wild are often non-object JSON ("30",42, booleans).src/api-client/api-client.ts): unchanged. Investigation showed{ data: { data } }is JS shorthand producing the correct{ data: <param> }wire body; the original bug was upstream. Added a regression test that pins the wire shape so a future refactor can't reintroduce the symptom.Test plan
npx vitest run src/core/platformconfig src/commands/platformconfig src/api-client— 10/10 platform-config tests + the api-client regression guard passnpx vitest run(full suite) — 2382/2382 pass, 4 skippednpx tsc --noEmit— cleandev-1: GET→PUT round-trip on configs22(experiment_form_max_secondary_metrics) and23(experiment_form_max_guardrail_metrics); both returned 200,updated_atadvanced, all fields preserved--show-requestoutput confirms the wire body is{"data": {"name": "...", "value": "30", "default_value": ..., ...}}Summary by CodeRabbit
Bug Fixes
New Behaviour
Tests