Add rowid-based selection support to fmodata mutations and docs#214
Open
Add rowid-based selection support to fmodata mutations and docs#214
Conversation
🦋 Changeset detectedLatest commit: 7906ee8 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.
|
📝 WalkthroughWalkthroughThis PR adds FileMaker Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/fmodata/tests/navigate.test.ts (1)
93-98: Optional: replace inlineROWIDliteral with a named constant.
Line 95 can be a bit clearer/maintainable with a constant likeROW_ID.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/fmodata/tests/navigate.test.ts` around lines 93 - 98, Replace the inline ROWID literal with a named constant to improve clarity and maintainability: define a constant (e.g., ROW_ID) near the top of the test (or in the test scope) and use it in the call to get({ ROWID: ROW_ID }) inside the test that builds queryBuilder for contacts -> users; update the expect to use the same constant so the test continues to assert "/contacts(ROWID=2)/users?$select=name" while avoiding the magic number in get({ ROWID: 2 }).packages/fmodata/tests/update.test.ts (1)
156-162: Optional: extract repeatedROWIDliteral into a constant for test clarity.
Using a shared constant instead of inline2in both new tests will improve readability and reduce duplication.As per coding guidelines, "Use meaningful variable names instead of magic numbers - extract constants with descriptive names".
Also applies to: 199-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/tests/update.test.ts` around lines 156 - 162, Extract the repeated magic number ROWID used in tests into a descriptive constant (e.g., const ROWID = 2) and replace inline uses of 2 in the tests that call db.from(users).update(...).byRowId(2) and any other occurrences (including the similar assertions around lines 199-209) with that constant; update the expectations accordingly so tests still assert an ExecutableUpdateBuilder from byRowId(ROWID) and any comparisons use ROWID for clarity and reduced duplication.packages/fmodata/tests/typescript.test.ts (1)
52-57: Optional: use a namedROWIDconstant in this ergonomics test.
This keeps intent explicit and aligns with the same pattern used across related test files.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/fmodata/tests/typescript.test.ts` around lines 52 - 57, Replace the magic number used for the ROWID call with a descriptive constant to match other tests: define a named constant (e.g., ROWID = 2 or ROWID_INDEX) at the top of the test and use it in the table.get call so that the assertions referencing getBuilder and rowIdBuilder (from table.get and getBuilder.getRequestConfig) read with explicit intent and follow the project pattern.packages/fmodata/tests/delete.test.ts (1)
52-58: Optional: centralize testROWIDvalue into a constant.
Both new test blocks hardcode2; using one named constant will make updates easier and keep intent explicit.As per coding guidelines, "Use meaningful variable names instead of magic numbers - extract constants with descriptive names".
Also applies to: 101-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/tests/delete.test.ts` around lines 52 - 58, Extract the hardcoded row id (the literal 2) used in the delete tests into a single descriptive constant (e.g., TEST_ROW_ID or SAMPLE_ROW_ID) and use that constant in both places where byRowId(2) appears; update the tests referencing db.from(usersTO).delete().byRowId(...) and any related assertions that use the same literal so they all reference the new constant (keeps the tests DRY and makes intent explicit).packages/fmodata/tests/record-builder-select-expand.test.ts (1)
168-173: Optional: extract theROWIDliteral to a named constant.
Line 169 uses a raw2; a constant (e.g.,const ROW_ID = 2) improves intent and consistency across tests.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/fmodata/tests/record-builder-select-expand.test.ts` around lines 168 - 173, Extract the magic number 2 used in the test "should support ROWID locators in get()" into a named constant (for example ROW_ID = 2) and use that constant in the call db.from(contacts).get({ ROWID: ROW_ID }).getQueryString(); update the expect assertions if they reference the literal directly to use the constant where applicable to improve readability and consistency.packages/fmodata/src/client/query/url-builder.ts (1)
127-144: Consider extracting shared navigation URL logic.There's noticeable duplication between
buildPathandbuildRecordNavigationfor constructing the base URL withsourceTable,baseRelation, andrecordLocator. Consider extracting this into a shared private helper to reduce repetition.The redundant
recordLocator === undefinedcheck (lines 138-140) can never be true since line 127 already guaranteesrecordLocatoris defined when entering this branch.♻️ Optional: Extract shared logic
+ private buildRecordNavigationBase( + navigation: NavigationConfig, + useEntityIds: boolean, + ): string { + const sourceTable = useEntityIds + ? (navigation.sourceTableEntityId ?? navigation.sourceTableName) + : navigation.sourceTableName; + const baseRelation = useEntityIds + ? (navigation.baseRelationEntityId ?? navigation.baseRelation) + : navigation.baseRelation; + const { recordLocator } = navigation; + if (recordLocator === undefined) { + throw new Error("recordLocator is required for record navigation"); + } + return baseRelation + ? buildRecordPath(`${sourceTable}/${baseRelation}`, recordLocator) + : buildRecordPath(sourceTable, recordLocator); + }Then use this helper in both
buildRecordNavigationandbuildPath.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/query/url-builder.ts` around lines 127 - 144, Extract the duplicated logic that builds the base navigation segment from buildPath and buildRecordNavigation into a private helper (e.g., buildNavigationBase or buildRecordBasePath) which takes parameters effectiveUseEntityIds, navigation (or its sourceTable, baseRelation, and recordLocator) and returns the base path string (using buildRecordPath when baseRelation is present); update both buildPath and buildRecordNavigation to call this helper to construct the base and then append the relation and queryString as before, and remove the redundant recordLocator undefined check inside buildRecordNavigation since the outer branch already guarantees its presence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/docs/src/app/docs/`(docs)/layout.tsx:
- Line 19: The footer anchor using href="http://proof.sh" should use HTTPS;
update the anchor element (the <a> with className "underline" and
target="_blank") in layout.tsx to use "https://proof.sh" so the link is loaded
securely and avoids insecure redirects or content tampering.
In `@apps/docs/src/app/docs/templates/layout.tsx`:
- Line 89: Update the external footer anchor in layout.tsx to use HTTPS by
changing the href value on the anchor element (the <a className="underline" ...>
tag) from "http://proof.sh" to "https://proof.sh"; keep the existing
rel="noopener noreferrer" and target="_blank" attributes intact to preserve
security and behavior.
---
Nitpick comments:
In `@packages/fmodata/src/client/query/url-builder.ts`:
- Around line 127-144: Extract the duplicated logic that builds the base
navigation segment from buildPath and buildRecordNavigation into a private
helper (e.g., buildNavigationBase or buildRecordBasePath) which takes parameters
effectiveUseEntityIds, navigation (or its sourceTable, baseRelation, and
recordLocator) and returns the base path string (using buildRecordPath when
baseRelation is present); update both buildPath and buildRecordNavigation to
call this helper to construct the base and then append the relation and
queryString as before, and remove the redundant recordLocator undefined check
inside buildRecordNavigation since the outer branch already guarantees its
presence.
In `@packages/fmodata/tests/delete.test.ts`:
- Around line 52-58: Extract the hardcoded row id (the literal 2) used in the
delete tests into a single descriptive constant (e.g., TEST_ROW_ID or
SAMPLE_ROW_ID) and use that constant in both places where byRowId(2) appears;
update the tests referencing db.from(usersTO).delete().byRowId(...) and any
related assertions that use the same literal so they all reference the new
constant (keeps the tests DRY and makes intent explicit).
In `@packages/fmodata/tests/navigate.test.ts`:
- Around line 93-98: Replace the inline ROWID literal with a named constant to
improve clarity and maintainability: define a constant (e.g., ROW_ID) near the
top of the test (or in the test scope) and use it in the call to get({ ROWID:
ROW_ID }) inside the test that builds queryBuilder for contacts -> users; update
the expect to use the same constant so the test continues to assert
"/contacts(ROWID=2)/users?$select=name" while avoiding the magic number in get({
ROWID: 2 }).
In `@packages/fmodata/tests/record-builder-select-expand.test.ts`:
- Around line 168-173: Extract the magic number 2 used in the test "should
support ROWID locators in get()" into a named constant (for example ROW_ID = 2)
and use that constant in the call db.from(contacts).get({ ROWID: ROW_ID
}).getQueryString(); update the expect assertions if they reference the literal
directly to use the constant where applicable to improve readability and
consistency.
In `@packages/fmodata/tests/typescript.test.ts`:
- Around line 52-57: Replace the magic number used for the ROWID call with a
descriptive constant to match other tests: define a named constant (e.g., ROWID
= 2 or ROWID_INDEX) at the top of the test and use it in the table.get call so
that the assertions referencing getBuilder and rowIdBuilder (from table.get and
getBuilder.getRequestConfig) read with explicit intent and follow the project
pattern.
In `@packages/fmodata/tests/update.test.ts`:
- Around line 156-162: Extract the repeated magic number ROWID used in tests
into a descriptive constant (e.g., const ROWID = 2) and replace inline uses of 2
in the tests that call db.from(users).update(...).byRowId(2) and any other
occurrences (including the similar assertions around lines 199-209) with that
constant; update the expectations accordingly so tests still assert an
ExecutableUpdateBuilder from byRowId(ROWID) and any comparisons use ROWID for
clarity and reduced duplication.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22534d7d-5d18-4f20-ab20-df5ffe2be6a8
📒 Files selected for processing (21)
.changeset/fresh-falcons-grow.mdapps/docs/content/docs/fmodata/crud.mdxapps/docs/content/docs/fmodata/extra-properties.mdxapps/docs/content/docs/fmodata/methods.mdxapps/docs/src/app/(home)/page.tsxapps/docs/src/app/docs/(docs)/layout.tsxapps/docs/src/app/docs/templates/layout.tsxpackages/fmodata/README.mdpackages/fmodata/skills/fmodata-client/SKILL.mdpackages/fmodata/src/client/builders/mutation-helpers.tspackages/fmodata/src/client/delete-builder.tspackages/fmodata/src/client/entity-set.tspackages/fmodata/src/client/query/url-builder.tspackages/fmodata/src/client/record-builder.tspackages/fmodata/src/client/update-builder.tspackages/fmodata/tests/delete.test.tspackages/fmodata/tests/mutation-helpers.test.tspackages/fmodata/tests/navigate.test.tspackages/fmodata/tests/record-builder-select-expand.test.tspackages/fmodata/tests/typescript.test.tspackages/fmodata/tests/update.test.ts
| rel="noopener" | ||
| target="_blank" | ||
| > | ||
| <a className="underline" href="http://proof.sh" rel="noopener" target="_blank"> |
There was a problem hiding this comment.
Use HTTPS here as well.
This footer link also uses http://proof.sh. Please switch to HTTPS to avoid insecure redirects/content tampering.
Suggested fix
- <a className="underline" href="http://proof.sh" rel="noopener" target="_blank">
+ <a className="underline" href="https://proof.sh" rel="noopener" target="_blank">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <a className="underline" href="http://proof.sh" rel="noopener" target="_blank"> | |
| <a className="underline" href="https://proof.sh" rel="noopener" target="_blank"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/docs/src/app/docs/`(docs)/layout.tsx at line 19, The footer anchor using
href="http://proof.sh" should use HTTPS; update the anchor element (the <a> with
className "underline" and target="_blank") in layout.tsx to use
"https://proof.sh" so the link is loaded securely and avoids insecure redirects
or content tampering.
| rel="noopener noreferrer" | ||
| target="_blank" | ||
| > | ||
| <a className="underline" href="http://proof.sh" rel="noopener noreferrer" target="_blank"> |
There was a problem hiding this comment.
Use HTTPS for the external footer link.
href="http://proof.sh" downgrades transport security and allows interception/redirect risks. Switch to HTTPS.
Suggested fix
- <a className="underline" href="http://proof.sh" rel="noopener noreferrer" target="_blank">
+ <a className="underline" href="https://proof.sh" rel="noopener noreferrer" target="_blank">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <a className="underline" href="http://proof.sh" rel="noopener noreferrer" target="_blank"> | |
| <a className="underline" href="https://proof.sh" rel="noopener noreferrer" target="_blank"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/docs/src/app/docs/templates/layout.tsx` at line 89, Update the external
footer anchor in layout.tsx to use HTTPS by changing the href value on the
anchor element (the <a className="underline" ...> tag) from "http://proof.sh" to
"https://proof.sh"; keep the existing rel="noopener noreferrer" and
target="_blank" attributes intact to preserve security and behavior.
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
rowidselection support across fmodata mutation/query buildersTesting
Summary by CodeRabbit
New Features
ROWIDrecord locators to fetch, update, and delete individual records directly viaget({ ROWID: n }),update().byRowId(n), anddelete().byRowId(n).Documentation
Tests
Style