fix(database-integrations): update apiIntegrationSchema to allow nullish values for optional fields - #453
Conversation
…ish values for optional fields
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe integration schema now permits omitted or null values for four metadata fields. API base URLs now normalize trailing slashes before endpoint construction. Integration requests now use a 30-second abort timeout. Tests cover the abort signal and normalized endpoint. Estimated code review effort: 2 (Simple) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #453 +/- ##
=======================================
Coverage 88.23% 88.24%
=======================================
Files 191 191
Lines 10688 10691 +3
Branches 2993 3077 +84
=======================================
+ Hits 9431 9434 +3
Misses 1255 1255
Partials 2 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/database-integrations/src/loading/fetch-integrations.ts (1)
13-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd nullish regression cases.
The supplied tests populate all four fields. Add cases that omit each field and set each field to
null. This protects the intended contract foris_public,created_at,updated_at, andfederated_auth_method.🤖 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/database-integrations/src/loading/fetch-integrations.ts` around lines 13 - 16, Add regression tests for the schema fields is_public, created_at, updated_at, and federated_auth_method, covering both omission and explicit null values for each. Keep the existing populated-field cases and verify all eight nullish inputs are accepted.
🤖 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 `@packages/database-integrations/src/loading/fetch-integrations.ts`:
- Around line 13-16: Add regression tests for the schema fields is_public,
created_at, updated_at, and federated_auth_method, covering both omission and
explicit null values for each. Keep the existing populated-field cases and
verify all eight nullish inputs are accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38e1c215-c60a-4a42-b3db-54e82bd9ad27
📒 Files selected for processing (1)
packages/database-integrations/src/loading/fetch-integrations.ts
`fetchIntegrations` concatenated the base URL without normalizing, so a `--url` with a trailing slash produced a double-slashed path that gateways answer with an unexplained 404. The fetch also had no `AbortSignal`, unlike every other API client in the monorepo, so a stalled API hung `deepnote run` and `integrations pull` indefinitely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ZoxDFVdqrAaa8EWfaVhFH
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/database-integrations/src/loading/fetch-integrations.ts (1)
13-16: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd tests for nullish metadata.
The provided test changes do not exercise this schema change. Add cases where each field is omitted and where each field is
null. Assert thatfetchIntegrationsaccepts both responses.As per coding guidelines, tests must cover new features and edge cases.
🤖 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/database-integrations/src/loading/fetch-integrations.ts` around lines 13 - 16, Add tests for fetchIntegrations covering the nullable metadata fields is_public, created_at, updated_at, and federated_auth_method: verify responses with each field omitted and with each field set to null are accepted by the schema.Source: Coding guidelines
packages/database-integrations/src/loading/fetch-integrations.test.ts (1)
52-54: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the 30-second timeout.
expect.any(AbortSignal)only proves that a signal exists. It does not detect an incorrect timeout value or an unbounded signal. Spy onAbortSignal.timeoutand assert30_000, or assert that the captured signal aborts after 30 seconds.As per coding guidelines, tests must cover new features and edge cases.
🤖 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/database-integrations/src/loading/fetch-integrations.test.ts` around lines 52 - 54, Update the test around the fetch call and its signal assertion to verify the 30-second timeout, not merely that an AbortSignal exists. Spy on AbortSignal.timeout and assert it is called with 30_000, or validate that the captured signal aborts after 30 seconds while preserving the existing fetch expectations.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.
Nitpick comments:
In `@packages/database-integrations/src/loading/fetch-integrations.test.ts`:
- Around line 52-54: Update the test around the fetch call and its signal
assertion to verify the 30-second timeout, not merely that an AbortSignal
exists. Spy on AbortSignal.timeout and assert it is called with 30_000, or
validate that the captured signal aborts after 30 seconds while preserving the
existing fetch expectations.
In `@packages/database-integrations/src/loading/fetch-integrations.ts`:
- Around line 13-16: Add tests for fetchIntegrations covering the nullable
metadata fields is_public, created_at, updated_at, and federated_auth_method:
verify responses with each field omitted and with each field set to null are
accepted by the schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a348894c-03c2-4c4e-a944-bf03e79a45c4
📒 Files selected for processing (2)
packages/database-integrations/src/loading/fetch-integrations.test.tspackages/database-integrations/src/loading/fetch-integrations.ts
Summary by CodeRabbit
null.