fix: respect caller Accept header in fmodata; better typegen metadata error#220
Merged
fix: respect caller Accept header in fmodata; better typegen metadata error#220
Conversation
_makeRequestEffect overwrote caller Accept; getMetadata({format:"xml"})
got json back, mis-cast to string, broke typegen parseMetadata.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Detect empty/JSON/HTML responses and include 500-char excerpt instead of opaque "No Edmx element found in XML". Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: c031d74 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
commit: |
📝 WalkthroughWalkthroughThis pull request fixes two separate issues: the Accept header in FileMaker OData requests is now preserved when explicitly set by callers, and metadata parsing error messages now include payload excerpts and identify common failure modes instead of a generic error. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 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 (1)
packages/typegen/src/fmodata/parseMetadata.ts (1)
76-77: Extract the HTML-detection window into a named constant.
200is a magic number here; giving it a descriptive constant will keep this heuristic easier to tune.♻️ Proposed tidy-up
const RESPONSE_EXCERPT_LIMIT = 500; +const HTML_DETECTION_PREFIX_LIMIT = 200; @@ - const lower = trimmed.slice(0, 200).toLowerCase(); + const lower = trimmed.slice(0, HTML_DETECTION_PREFIX_LIMIT).toLowerCase();As per coding guidelines, "Use meaningful variable names instead of magic numbers - extract constants with descriptive names".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typegen/src/fmodata/parseMetadata.ts` around lines 76 - 77, Extract the magic number 200 into a named constant (e.g., HTML_DETECTION_WINDOW = 200) and replace trimmed.slice(0, 200) with trimmed.slice(0, HTML_DETECTION_WINDOW) so the HTML-detection heuristic is configurable and self-documenting; update the code near parseMetadata (where `trimmed` is sliced into `lower`) and ensure the new constant is declared with an appropriate scope (module-level or function-level) and a descriptive name like HTML_DETECTION_WINDOW.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/typegen/src/fmodata/parseMetadata.ts`:
- Around line 76-77: Extract the magic number 200 into a named constant (e.g.,
HTML_DETECTION_WINDOW = 200) and replace trimmed.slice(0, 200) with
trimmed.slice(0, HTML_DETECTION_WINDOW) so the HTML-detection heuristic is
configurable and self-documenting; update the code near parseMetadata (where
`trimmed` is sliced into `lower`) and ensure the new constant is declared with
an appropriate scope (module-level or function-level) and a descriptive name
like HTML_DETECTION_WINDOW.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c91ca9c5-bc86-4faa-9b1c-64300d26059a
📒 Files selected for processing (4)
.changeset/fix-fmodata-respect-caller-accept-header.md.changeset/improve-typegen-metadata-error.mdpackages/fmodata/src/client/filemaker-odata.tspackages/typegen/src/fmodata/parseMetadata.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
_makeRequestEffect(filemaker-odata.ts) unconditionally setAccept: application/json, clobbering theAccept: application/xmlthatgetMetadata({ format: "xml" })passed in. Server returned JSON, the JSON branch parsed it into an object, that object was cast to `string` and handed to fast-xml-parser → `@proofkit/typegen` blew up with "No Edmx element found in XML" on every fmodata config. Fix: only apply the default Accept when the caller hasn't supplied one.Repro: any `@proofkit/typegen` config with `type: "fmodata"` against current beta.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes