[OSDEV-2368] Add reusable labels, default value fallback, and external link icons to partner fields#912
Open
protsack-stephan wants to merge 7 commits intomainfrom
Open
[OSDEV-2368] Add reusable labels, default value fallback, and external link icons to partner fields#912protsack-stephan wants to merge 7 commits intomainfrom
protsack-stephan wants to merge 7 commits intomainfrom
Conversation
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 43.64%Your code coverage diff: 0.17% ▴ ✅ All code changes are covered |
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 55.73%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.75%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Replace inline label spans and the shared `label` class in commonPropertyStyles with a dedicated PartnerFieldLabel component that encapsulates the styling, removing the need to pass a custom class through withStyles. Made-with: Cursor
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Django App | Unittest test suite - Code coverage reportTotal: 81.98%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
…partner field links Partner field links (URI, URL, and URI Reference properties) now display a small external-link icon next to the link text, so users can tell at a glance that the link opens in a new tab. Partners can also specify custom link text via a "text" field in their schema configuration. When provided, this text is shown instead of the raw URL, making links more readable and user-friendly. Made-with: Cursor
protsack-stephan
changed the title
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
protsack-stephan
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository 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)
📝 WalkthroughWalkthroughIntroduces a reusable PartnerFieldLabel component, applies it across partner field property components, adds schema-default fallbacks for missing values, appends external-link icons to link properties, and updates utilities/tests to support custom link text resolution via schema. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/react/src/components/PartnerFields/FormatComponents/UriReferenceProperty/UriReferenceProperty.jsx (1)
26-31:⚠️ Potential issue | 🟠 MajorSchema defaults still won't render for
uri-referencefields.Passing
propertyValueas the fallback here does not help becausepropertyValueis still read only fromvalue[propertyKey]. When the key is absent, Line 19 returnsnullbeforeschemaProperties[propertyKey].defaultis ever consulted, so this component misses the new default-value behavior.Proposed fix
const UriReferenceProperty = ({ propertyKey, value, schemaProperties: incomingSchemaProperties, partnerConfigFields: incomingPartnerConfigFields, classes, }) => { - const propertyValue = value[propertyKey]; + const schemaProperties = incomingSchemaProperties || {}; + const schemaProperty = schemaProperties[propertyKey] || {}; + const propertyValue = Object.prototype.hasOwnProperty.call(value, propertyKey) + ? value[propertyKey] + : schemaProperty.default; if (!propertyValue) { return null; } - const schemaProperties = incomingSchemaProperties || {}; const partnerConfigFields = incomingPartnerConfigFields || {}; const description = getDescription(propertyKey, schemaProperties);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/components/PartnerFields/FormatComponents/UriReferenceProperty/UriReferenceProperty.jsx` around lines 26 - 31, The issue is that UriReferenceProperty passes propertyValue (which is derived from value[propertyKey]) as the fallback to getLinkTextFromSchema, preventing schemaProperties[propertyKey].default from being used when the key is absent; change the call so the fallback is the schema default instead of propertyValue by replacing the last argument with schemaProperties[propertyKey]?.default (use optional chaining to avoid crashes) so getLinkTextFromSchema can fall back to the schema default when value[propertyKey] is missing.
🧹 Nitpick comments (2)
src/react/src/__tests__/components/UriReferenceProperty.test.js (2)
95-96: Redundant assertion can be simplified.
screen.getByText('local-id')already asserts the element exists with that text content. The subsequenttoHaveTextContentassertion is redundant.♻️ Suggested simplification
- const text = screen.getByText('local-id'); - expect(text).toHaveTextContent('local-id'); + expect(screen.getByText('local-id')).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/__tests__/components/UriReferenceProperty.test.js` around lines 95 - 96, The test contains a redundant assertion: calling screen.getByText('local-id') already verifies an element with that text exists, so remove the extra expect(...).toHaveTextContent('local-id') in UriReferenceProperty.test.js; keep the screen.getByText('local-id') call (or replace it with const el = screen.getByText('local-id') and assert on el only if you need further checks) to simplify the test and avoid duplicate assertions.
114-115: Same redundant assertion pattern.♻️ Suggested simplification
- const text = screen.getByText('plain-value'); - expect(text).toHaveTextContent('plain-value'); + expect(screen.getByText('plain-value')).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/src/__tests__/components/UriReferenceProperty.test.js` around lines 114 - 115, The test contains a redundant assertion: calling screen.getByText('plain-value') already throws if the node is missing, so remove the duplicate expect(text).toHaveTextContent('plain-value') and replace it with a single, clear assertion such as asserting presence (e.g., expect(screen.getByText('plain-value')).toBeInTheDocument()) or keep only the getByText call; update the test in UriReferenceProperty.test.js by removing the extra expect that references the local variable text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agent/skills/pr-description/SKILL.md:
- Around line 90-92: Update the "good" example sentence in SKILL.md that
currently reads "using this as in Python3": replace it with "using this in
Python 3" and normalize all occurrences in that same block to use consistent
spacing and capitalization for "Python 2" and "Python 3" (e.g., "Python 2" /
"Python 3"); specifically edit the example text following "Create a Python3
build rule for status.py." so the wording is clear and consistent.
- Around line 22-23: Update the example diff commands in SKILL.md to use the
three-dot form so PR-scoped changes are shown; replace instances of "git diff
<base-branch>..HEAD --stat" and "git diff <base-branch>..HEAD" with the
merge-base scoped versions "git diff <base-branch>...HEAD --stat" and "git diff
<base-branch>...HEAD" respectively, and update any surrounding explanatory text
to mention that the three-dot form compares from the merge-base to show only
changes introduced by the branch.
In `@src/react/src/__tests__/utils/PartnerFieldSchemaValueUtils.test.js`:
- Around line 155-156: The test currently uses a concise arrow callback that
returns the matcher from result.forEach(el =>
expect(React.isValidElement(el)).toBe(true)); — change the callback to a block
body so it does not return the matcher: update the call to result.forEach(el =>
{ expect(React.isValidElement(el)).toBe(true); }); ensuring you modify the
forEach callback in PartnerFieldSchemaValueUtils.test.js and keep the assertion
using React.isValidElement unchanged.
In `@src/react/src/components/PartnerFields/FormatComponents/DateProperty.jsx`:
- Around line 10-17: The code assumes schemaProperties[propertyKey] exists and
dereferences schemaProperty.default, which can throw if undefined; update the
logic in DateProperty.jsx around schemaProperties/propertyKey to guard against
missing schema entries by resolving schemaProperty =
schemaProperties[propertyKey] || {} (or use optional chaining) and pass a safe
default when calling getFormattedDateValue (e.g., use schemaProperty.default ??
undefined or wrap the second getFormattedDateValue argument with {
[propertyKey]: schemaProperty?.default } so you never access .default on
undefined); ensure both calls to getFormattedDateValue handle the safe fallback.
In `@src/react/src/components/PartnerFields/FormatComponents/UriProperty.jsx`:
- Around line 11-12: The current assignment uses value[propertyKey] ||
schemaProperty.default which treats any falsy value (e.g., empty string, 0,
false) as missing; change it to check for key presence instead: determine
schemaProperty as now (schemaProperties[propertyKey] || {}) and set
propertyValue by testing whether value actually contains propertyKey (e.g.,
using Object.prototype.hasOwnProperty.call(value, propertyKey) or propertyKey in
value) and only use schemaProperty.default when the key is absent; update
references to schemaProperty, propertyKey, propertyValue, and value accordingly.
In `@src/react/src/components/PartnerFields/FormatComponents/UrlProperty.jsx`:
- Around line 11-12: Replace the fallback that uses || with a presence check so
explicit falsy values (e.g. empty string) are preserved: in the UrlProperty
component compute propertyValue by checking whether value has the propertyKey
(e.g. Object.prototype.hasOwnProperty.call(value, propertyKey) or value != null
&& propertyKey in value) and only use schemaProperty.default when the key is
missing; ensure you guard for value being null/undefined before testing
presence.
In `@src/react/src/components/PartnerFields/PartnerFieldSchemaValue/utils.jsx`:
- Around line 97-98: The guard incorrectly treats falsy schema defaults (0,
false, '', etc.) as absent; update the conditional that uses
propertySchema.default to instead check for the presence of the default key
(e.g., use 'default' in propertySchema or
Object.prototype.hasOwnProperty.call(propertySchema, 'default')) so properties
with falsy defaults are preserved when propertyKey is not in value; locate the
check around propertySchema = schemaProperties[propertyKey] || {} and replace
the && !propertySchema.default test with a presence check for the default key.
In
`@src/react/src/components/PartnerFields/TypeComponents/DefaultProperty/DefaultProperty.jsx`:
- Around line 11-13: The current fallback uses || which treats 0, false, and ''
as missing and substitutes schemaProperty.default; change the logic in
DefaultProperty.jsx so stringValue uses a nullish check instead (e.g., replace
the || fallback with ?? or an explicit check) so
getPropertyValueAsString(propertyKey, value) is only replaced by
schemaProperty.default when it returns null or undefined; reference symbols:
schemaProperty, stringValue, getPropertyValueAsString(propertyKey, value), and
schemaProperty.default.
In `@src/react/src/components/PartnerFields/TypeComponents/IntegerProperty.jsx`:
- Around line 10-11: The code uses the || operator when computing propertyValue,
which treats numeric 0 as falsy and thus overrides a valid 0 with
schemaProperty.default; update the propertyValue assignment (the expression
referencing propertyValue, value[propertyKey], and schemaProperty.default) to
use a nullish check (e.g., use the nullish coalescing operator ?? or an explicit
undefined/hasOwnProperty check) so that 0 is preserved as a valid value while
still falling back to schemaProperty.default only when the value is null or
undefined.
---
Outside diff comments:
In
`@src/react/src/components/PartnerFields/FormatComponents/UriReferenceProperty/UriReferenceProperty.jsx`:
- Around line 26-31: The issue is that UriReferenceProperty passes propertyValue
(which is derived from value[propertyKey]) as the fallback to
getLinkTextFromSchema, preventing schemaProperties[propertyKey].default from
being used when the key is absent; change the call so the fallback is the schema
default instead of propertyValue by replacing the last argument with
schemaProperties[propertyKey]?.default (use optional chaining to avoid crashes)
so getLinkTextFromSchema can fall back to the schema default when
value[propertyKey] is missing.
---
Nitpick comments:
In `@src/react/src/__tests__/components/UriReferenceProperty.test.js`:
- Around line 95-96: The test contains a redundant assertion: calling
screen.getByText('local-id') already verifies an element with that text exists,
so remove the extra expect(...).toHaveTextContent('local-id') in
UriReferenceProperty.test.js; keep the screen.getByText('local-id') call (or
replace it with const el = screen.getByText('local-id') and assert on el only if
you need further checks) to simplify the test and avoid duplicate assertions.
- Around line 114-115: The test contains a redundant assertion: calling
screen.getByText('plain-value') already throws if the node is missing, so remove
the duplicate expect(text).toHaveTextContent('plain-value') and replace it with
a single, clear assertion such as asserting presence (e.g.,
expect(screen.getByText('plain-value')).toBeInTheDocument()) or keep only the
getByText call; update the test in UriReferenceProperty.test.js by removing the
extra expect that references the local variable text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a73eb2c8-f08a-424d-8d3d-2f306bb55578
📒 Files selected for processing (24)
.agent/skills/pr-description/SKILL.mddoc/release/RELEASE-NOTES.mdsrc/react/src/__tests__/components/DateProperty.test.jssrc/react/src/__tests__/components/DateTimeProperty.test.jssrc/react/src/__tests__/components/DefaultProperty.test.jssrc/react/src/__tests__/components/PartnerFieldLabel.test.jsxsrc/react/src/__tests__/components/PartnerFieldSchemaValue.test.jssrc/react/src/__tests__/components/UriProperty.test.jssrc/react/src/__tests__/components/UriReferenceProperty.test.jssrc/react/src/__tests__/components/UrlProperty.test.jssrc/react/src/__tests__/utils/PartnerFieldSchemaValueUtils.test.jssrc/react/src/__tests__/utils/PartnerFieldsUtils.test.jssrc/react/src/components/PartnerFields/FormatComponents/DateProperty.jsxsrc/react/src/components/PartnerFields/FormatComponents/DateTimeProperty.jsxsrc/react/src/components/PartnerFields/FormatComponents/UriProperty.jsxsrc/react/src/components/PartnerFields/FormatComponents/UriReferenceProperty/UriReferenceProperty.jsxsrc/react/src/components/PartnerFields/FormatComponents/UrlProperty.jsxsrc/react/src/components/PartnerFields/PartnerFieldLabel/PartnerFieldLabel.jsxsrc/react/src/components/PartnerFields/PartnerFieldLabel/styles.jssrc/react/src/components/PartnerFields/PartnerFieldSchemaValue/utils.jsxsrc/react/src/components/PartnerFields/TypeComponents/DefaultProperty/DefaultProperty.jsxsrc/react/src/components/PartnerFields/TypeComponents/IntegerProperty.jsxsrc/react/src/components/PartnerFields/styles.jssrc/react/src/components/PartnerFields/utils.js
src/react/src/components/PartnerFields/FormatComponents/DateProperty.jsx
Outdated
Show resolved
Hide resolved
src/react/src/components/PartnerFields/PartnerFieldSchemaValue/utils.jsx
Show resolved
Hide resolved
src/react/src/components/PartnerFields/TypeComponents/DefaultProperty/DefaultProperty.jsx
Show resolved
Hide resolved
src/react/src/components/PartnerFields/TypeComponents/IntegerProperty.jsx
Show resolved
Hide resolved
|
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
2 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.
Partner field property components had duplicated inline label rendering, no support for schema-defined default values, and no visual indicator that links open externally.
PartnerFieldLabelcomponent - Replaces inline title logic acrossDateProperty,DateTimeProperty,UriProperty,UrlProperty,DefaultProperty, andIntegerPropertywith a dedicated styled component.defaultwhen the value object is missing the key.UriProperty,UrlProperty, andUriReferencePropertyrender anOpenInNewIconnext to link text.getLinkTextFromSchemasupports atextfield on schema properties for human-readable link labels.Tests added or updated for all affected components.
OSDEV-2368