feat(serve): add strict POST /merge endpoint#240
Closed
hfl0506 wants to merge 0 commit into
Closed
Conversation
📝 WalkthroughWalkthroughAdds a ChangesServe REST API
Integer chart flags
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ServeAPI
participant ContractDecoder
participant Core
Client->>ServeAPI: POST /, /merge, or /ui
ServeAPI->>ContractDecoder: strict decode and validate JSON
ContractDecoder-->>ServeAPI: validated request or Problem+JSON details
ServeAPI->>Core: convert or merge datasets
Core-->>ServeAPI: datasets or processing error
ServeAPI-->>Client: JSON, HTML, or Problem+JSON response
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
hfl0506
marked this pull request as ready for review
July 19, 2026 00:53
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cmd/serve_test.go (1)
28-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the shared JSON fixtures on
ServeSuite.Move these payload fixtures onto the suite and initialize them in suite setup rather than maintaining package-level shared fixtures.
As per coding guidelines, “Put shared fixtures and setup on the Go test suite.”
🤖 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 `@cmd/serve_test.go` around lines 28 - 32, Move validDatasetJSON, firstMergeJSON, and secondMergeJSON from package-level constants onto ServeSuite, and initialize them during the suite setup lifecycle. Update all tests to reference the suite-owned fixture fields while preserving their existing payload values.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 `@api/openapi.yaml`:
- Around line 409-453: Complete the chart-config uniqueness contract by reusing
the existing per-type constraints from the shown configs schema in
ChartSelection.configs within api/openapi.yaml (anchor 409-453). Update
api/openapi_test.go (sibling 167-188) to reject duplicate configurations for
every supported chart type—bar, line, scatter, pie, heatmap, and radar—in both
Dataset and request schema locations.
In `@cmd/serve_test.go`:
- Around line 144-224: Update the lifecycle tests
TestCancellationShutsDownInMemoryListener,
TestCancellationTriggersGracefulShutdown, TestShutdownFailureIsReturned, and
TestServeFailureDuringShutdownIsReturned to replace direct receives from
acceptStarted and result with the suite’s timeout-aware assertion/helper. Ensure
both listener startup synchronization and runServer completion fail promptly
when they do not occur, while preserving the existing error and shutdown
assertions.
In `@cmd/serve.go`:
- Around line 85-90: Update composeRESTRoutes to add explicit fallback handling
for unmatched paths and method mismatches, routing both through writeAPIProblem
so responses use the documented application/problem+json format instead of
ServeMux plain text. Preserve the existing convert, merge, and ui routes, and
add route tests covering unknown paths and unsupported methods.
In `@docs/src/content/docs/commands/serve.mdx`:
- Around line 26-29: Update runServer’s shutdown handling so an error returned
by Shutdown cannot leave in-flight work running beyond the documented 10-second
bound. Ensure the shutdown context or timeout is enforced through the failure
path, while preserving graceful completion for work that finishes within the
limit.
---
Nitpick comments:
In `@cmd/serve_test.go`:
- Around line 28-32: Move validDatasetJSON, firstMergeJSON, and secondMergeJSON
from package-level constants onto ServeSuite, and initialize them during the
suite setup lifecycle. Update all tests to reference the suite-owned fixture
fields while preserving their existing payload values.
🪄 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
Run ID: d6b3f3ef-7984-4f93-872f-a408bd5958dd
📒 Files selected for processing (14)
api/openapi.yamlapi/openapi_test.gocmd/cli/flagbag.gocmd/cli/flagbag_test.gocmd/serve.gocmd/serve_contract.gocmd/serve_test.gocmd/testing.godocs/src/content/docs/commands/serve.mdxinternal/flags/flag.gopkg/core/core.gopkg/core/core_test.goshared/chart_spec.goshared/chart_spec_test.go
Comment on lines
+26
to
+29
| Each request body is limited to 10 MiB. The server uses a 5-second header read | ||
| timeout, a 15-second request read timeout, and 60-second write and idle | ||
| timeouts. On `SIGINT` or `SIGTERM`, it stops accepting requests and allows up | ||
| to 10 seconds for in-flight work to finish. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enforce the documented ten-second shutdown bound.
The current failure path in runServer can leave active work running after Shutdown returns an error, so “up to 10 seconds” is not presently guaranteed.
🤖 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 `@docs/src/content/docs/commands/serve.mdx` around lines 26 - 29, Update
runServer’s shutdown handling so an error returned by Shutdown cannot leave
in-flight work running beyond the documented 10-second bound. Ensure the
shutdown context or timeout is enforced through the failure path, while
preserving graceful completion for work that finishes within the limit.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/openapi.yaml (1)
374-421: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract duplicate chart-config constraints into a shared component.
The uniqueness constraints for chart configurations (the
allOf/containsblock) are perfectly implemented, but they are identical here and inDataset.settings(lines 451-497). Extracting this into a reusable schema component will eliminate duplication and ensure both arrays remain consistently validated if a new chart type is added in the future.♻️ Proposed refactoring
Add a new schema component, for example,
ChartConfigList:ChartConfigList: type: array description: Each chart type may appear at most once. items: $ref: '`#/components/schemas/ChartConfig`' allOf: - contains: type: object required: [type] properties: type: { const: bar } minContains: 0 maxContains: 1 - contains: type: object required: [type] properties: type: { const: line } minContains: 0 maxContains: 1 # ... (include all other chart types)Then reference it in both locations. In
ChartSelection:types: # ... - configs: - type: array - description: Each chart type may appear at most once. - items: - $ref: '`#/components/schemas/ChartConfig`' - allOf: - - contains: - type: object - required: [type] - properties: - type: { const: bar } - minContains: 0 - maxContains: 1 - # ... (remaining types) + configs: + $ref: '`#/components/schemas/ChartConfigList`'And similarly in
Dataset:axes: type: array items: { $ref: '`#/components/schemas/Axis`' } - settings: - type: array - description: Each chart type may appear at most once. - items: { $ref: '`#/components/schemas/ChartConfig`' } - allOf: - - contains: - type: object - required: [type] - properties: - type: { const: bar } - minContains: 0 - maxContains: 1 - # ... (remaining types) + settings: + $ref: '`#/components/schemas/ChartConfigList`'🤖 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 `@api/openapi.yaml` around lines 374 - 421, Extract the duplicated chart-type uniqueness constraints into a shared components schema, such as ChartConfigList, preserving its array description, ChartConfig items, and all existing contains/maxContains rules for every chart type. Replace the inline configs definitions in ChartSelection and Dataset.settings with references to this shared schema so both locations use the same validation.
🤖 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 `@api/openapi.yaml`:
- Around line 374-421: Extract the duplicated chart-type uniqueness constraints
into a shared components schema, such as ChartConfigList, preserving its array
description, ChartConfig items, and all existing contains/maxContains rules for
every chart type. Replace the inline configs definitions in ChartSelection and
Dataset.settings with references to this shared schema so both locations use the
same validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e05d981-e5eb-4cd4-a8ab-92cb013df6ba
📒 Files selected for processing (5)
api/openapi.yamlapi/openapi_test.gocmd/serve.gocmd/serve_test.godocs/src/content/docs/commands/serve.mdx
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/src/content/docs/commands/serve.mdx
- api/openapi_test.go
- cmd/serve.go
- cmd/serve_test.go
Member
|
Hi @hfl0506, Thanks for these PRs. You have direct access to this repo, so you don't need to create PRs from the forked one. You can clone this repo and start working on it. |
Collaborator
Author
Hi @fahimfaisaal do I need to recreate these MR or just starting from the new MR ? Thanks |
Member
|
@hfl0506 no, not for the current ones; you can follow it from the next PRs |
This was referenced Jul 19, 2026
hfl0506
force-pushed
the
feat/rest-merge-endpoint
branch
from
July 19, 2026 07:04
783e28c to
07d6aea
Compare
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
3 participants
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.
Related Issue
Closes #215
Changes
Added strict POST /merge handling with tagAxis support.
Preserved Dataset wire format and always returns an array.
Added nested strict validation and structured Problem Details
errors.
Added real merge-axis conflict detection in the shared core.
Expanded api/openapi.yaml with schemas and examples.
Added handler, behavior, core, and contract tests.
Test Result
Case 1
Case 3
Case 3
Case 4
Summary by CodeRabbit
vizb servelocal REST API for conversion, merging, and UI rendering.406handling and merge conflict examples.