feat: [GH-356] allow to use if with relations - #357
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates relation validation to handle async and non-async relationships more precisely, adds dummy-model coverage for presence validation on related records, and refreshes compatibility, dependency, and CI settings for the supported Ember version set. ChangesAsync relation validation
Compatibility and CI updates
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CoreValidator
participant ModelRelations
participant ValidationConfig
CoreValidator->>ValidationConfig: read relations / relations.value
CoreValidator->>ModelRelations: resolve relationship state
CoreValidator->>CoreValidator: check hasMany / belongsTo rules
CoreValidator-->>ValidationConfig: return validation result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
ixxvivxxi
changed the title
|
@ixxvivxxi Thank you for putting this together! Could you check of the failing test before we move ahead with this PR. |
|
@esbanarango Yes! I will fix it in a few days. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
package.json (1)
39-39: Fixed version specified for ember-cli-htmlbars.The dependency
ember-cli-htmlbarsis using a fixed version ("6.3.0") rather than a flexible version range (e.g.,^6.3.0). This could make future upgrades more difficult as it will not automatically accept compatible minor/patch versions.Consider using a caret range syntax for consistent dependency management:
- "ember-cli-htmlbars": "6.3.0", + "ember-cli-htmlbars": "^6.3.0",tests/dummy/config/ember-try.js (1)
3-3: Replaced dynamic channel URLs with fixed version scenarios.You've commented out
getChannelURLand dynamic test scenarios (release, beta, canary) in favor of fixed version scenarios. This approach provides more deterministic testing but may miss compatibility issues with upcoming Ember versions.Consider keeping at least one dynamic scenario (like release) to catch potential issues with the next version:
-// const getChannelURL = require('ember-source-channel-url'); +const getChannelURL = require('ember-source-channel-url'); // Later in the file: - // { - // name: 'ember-release', - // npm: { - // devDependencies: { - // 'ember-source': await getChannelURL('release'), - // 'ember-data': 'lts', - // }, - // }, - // }, + { + name: 'ember-release', + npm: { + devDependencies: { + 'ember-source': await getChannelURL('release'), + 'ember-data': 'lts', + }, + }, + },Also applies to: 46-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
README.md(1 hunks)package.json(1 hunks)tests/dummy/config/ember-try.js(2 hunks)tests/helpers/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/helpers/index.ts
🔇 Additional comments (4)
package.json (1)
110-111:Details
❓ Verification inconclusive
Added resolutions field for @ember/test-waiters.
Adding a resolutions field to fix
@ember/test-waitersat version 3.1.0 suggests there might be a compatibility issue with this package. Resolutions are typically used to solve version conflicts or dependency issues.
🏁 Script executed:
#!/bin/bash # Check for known issues with @ember/test-waiters in GitHub issues gh search issues --repo "emberjs/ember.js" --state open "@ember/test-waiters" --json title,url,state | jq '.'Length of output: 5373
Double-check the necessity of the forced version for @ember/test-waiters
The update to set the resolution for
@ember/test-waitersto"3.1.0"inpackage.json(lines 110–111) appears to be a precaution to avoid potential dependency conflicts. A search for known issues in the Ember.js repository did not return any direct problems related to this package version. However, since resolutions are usually applied to overcome version conflicts, please verify that this change is intentional and that no side effects occur (e.g., run your full test suite).
- Location:
package.json, lines 110–111- Observation: No open issues related to
@ember/test-waitersat 3.1.0 were found in the Ember.js GitHub issues.- Action: Confirm that the forced resolution is necessary and that all tests pass after this change.
README.md (2)
23-26: LGTM! Added clear compatibility information.The new compatibility section clearly documents the version requirements for this addon, which is excellent for users to understand what versions of Ember.js and Ember Data are supported.
25-25:Details
✅ Verification successful
Verify compatibility with ember-source 6.2.
The README states compatibility with ember-source up to version 6.2, but it would be good to confirm this has been thoroughly tested.
🏁 Script executed:
#!/bin/bash # Check if there are test scenarios for the stated maximum compatibility version grep -A 5 "ember-6.2" tests/dummy/config/ember-try.jsLength of output: 230
Compatibility verification for ember-source 6.2 confirmed
The test configuration in
tests/dummy/config/ember-try.jsincludes a scenario namedember-6.2-and-ember-data-5.3, which explicitly sets'ember-source': '~6.2.0'. This confirms that compatibility with ember-source 6.2 has been thoroughly tested as stated in the README.tests/dummy/config/ember-try.js (1)
29-33: Updated test scenarios to match compatibility requirements.The test scenarios have been updated to include specific versions mentioned in the README's compatibility section. This ensures that the addon is properly tested against the supported Ember versions.
Also applies to: 38-42
|
@esbanarango Hi! I was only able to get working tests up to |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
59-64: Review Updated Test Scenarios in the MatrixThe CI matrix now includes the new scenarios
"ember-lts-5.12-and-ember-data-5.3"and"ember-6.2-and-ember-data-5.3", which are aligned with the versions where tests currently pass. This adjustment avoids running tests for failing or unstable Ember versions while ensuring core functionality is verified.
65-67: Clarify Commented-Out ScenariosThe older scenarios (
ember-release,ember-beta, andember-canary) have been left commented out. Please ensure that this decision is documented—either via an inline comment or commit message—so that future maintainers understand these are being retained for potential reactivation once the issues are resolved.
|
@esbanarango Hi! I need to understand what happened, because yesterday I could run all tests, but today when I remove node_modules I get an error. |
Yeah something went off after locking ED version 🤔 . Check the output of this tests failing: https://github.com/esbanarango/ember-model-validator/actions/runs/14117761374/job/39552213633?pr=357 |
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
62-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd context for disabled try-scenarios.
The commented-out
ember-release,ember-beta, andember-canaryscenarios lack an explanation for why they're disabled. A brief comment referencing the compatibility constraint (tests only pass up to ember-source v6.2) would help future maintainers understand when to revisit re-enabling them.📝 Suggested comment
- ember-classic - # - ember-release - # - ember-beta - # - ember-canary + # Disabled: tests only pass up to ember-source v6.2 — see GH-356 + # - ember-release + # - ember-beta + # - ember-canary🤖 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 @.github/workflows/ci.yml around lines 62 - 67, The disabled try-scenarios in the CI workflow need an explanatory note so future maintainers know why they are commented out. Update the workflow near the ember-release, ember-beta, and ember-canary entries with a brief comment stating they remain disabled because the test suite only passes up to ember-source v6.2, and reference the existing scenario list in the workflow so it’s clear when to revisit re-enabling them.
🤖 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 `@addon/decorators/core-validator.js`:
- Around line 125-131: Cache the result of `_modelRelations()` in the validation
block so it is only evaluated once, then reuse that cached value for the blank
check and property lookup to avoid repeated `eachRelationship` iteration. Also
harden the `relationshipFor` access in `core-validator.js` by adding optional
chaining before `.options` so a missing relation does not throw, and use the
cached relation data in the `propertyValue` assignment path inside the same
conditional.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 62-67: The disabled try-scenarios in the CI workflow need an
explanatory note so future maintainers know why they are commented out. Update
the workflow near the ember-release, ember-beta, and ember-canary entries with a
brief comment stating they remain disabled because the test suite only passes up
to ember-source v6.2, and reference the existing scenario list in the workflow
so it’s clear when to revisit re-enabling them.
🪄 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: 7c9e17ae-816a-4e3a-b0d4-6871627ad8f2
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.github/workflows/ci.ymladdon/decorators/core-validator.jspackage.jsontests/dummy/app/models/async-model.tstests/unit/models/async-model-test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
| if (this._modelRelations() && !isBlank(this._modelRelations()[property])) { | ||
| propertyValue = get(this, `${property}.content`); | ||
| if (this['relationshipFor'](property).options?.async === false) { | ||
| propertyValue = this._modelRelations()[property]; | ||
| } else { | ||
| propertyValue = get(this, `${property}.content`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Cache _modelRelations() and guard against potential NPE.
_modelRelations() is called three times in this block; in the fallback branch each call iterates all relationships via eachRelationship. Additionally, this['relationshipFor'](property).options will throw a TypeError if relationshipFor returns null/undefined — the ?. only guards .async, not .options.
♻️ Proposed fix: cache result and add optional chaining
- if (this._modelRelations() && !isBlank(this._modelRelations()[property])) {
- if (this['relationshipFor'](property).options?.async === false) {
- propertyValue = this._modelRelations()[property];
+ const modelRelations = this._modelRelations();
+ if (modelRelations && !isBlank(modelRelations[property])) {
+ if (this['relationshipFor'](property)?.options?.async === false) {
+ propertyValue = modelRelations[property];
} else {
propertyValue = get(this, `${property}.content`);
}📝 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.
| if (this._modelRelations() && !isBlank(this._modelRelations()[property])) { | |
| propertyValue = get(this, `${property}.content`); | |
| if (this['relationshipFor'](property).options?.async === false) { | |
| propertyValue = this._modelRelations()[property]; | |
| } else { | |
| propertyValue = get(this, `${property}.content`); | |
| } | |
| } | |
| const modelRelations = this._modelRelations(); | |
| if (modelRelations && !isBlank(modelRelations[property])) { | |
| if (this['relationshipFor'](property)?.options?.async === false) { | |
| propertyValue = modelRelations[property]; | |
| } else { | |
| propertyValue = get(this, `${property}.content`); | |
| } | |
| } |
🤖 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 `@addon/decorators/core-validator.js` around lines 125 - 131, Cache the result
of `_modelRelations()` in the validation block so it is only evaluated once,
then reuse that cached value for the blank check and property lookup to avoid
repeated `eachRelationship` iteration. Also harden the `relationshipFor` access
in `core-validator.js` by adding optional chaining before `.options` so a
missing relation does not throw, and use the cached relation data in the
`propertyValue` assignment path inside the same conditional.
There was a problem hiding this comment.
Applied in 4639648:
cached _modelRelations() to avoid repeated lookups
added optional chaining on relationshipFor(property)?.options?.async
Also added a CI comment explaining that dynamic channel scenarios are replaced by pinned versions (including ember-lts-6.12-and-ember-data-5.8).
There was a problem hiding this comment.
@ixxvivxxi Thanks for confirming — the caching and optional chaining changes look correct in the current file. Appreciate the extra CI clarification too.
ᕕ( ᐛ )ᕗ
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Rebase feature changes onto the new v5 addon structure (pnpm, vite, src/) while preserving esbanarangoGH-356 relation validation fixes and async: false presence support. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Summary by CodeRabbit
hasManyandbelongsTorelationships.