-
Notifications
You must be signed in to change notification settings - Fork 39
fix: support normalized json array for attribute input #6738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for normalized JSON array input to the fieldsToIncludeInCitations property in the atomic-generated-answer component. The change allows the property to accept either a string array (new standard) or a comma-separated string (deprecated), maintaining backward compatibility while preparing for v4 where only array input will be supported.
Key changes:
- Property type updated from
stringtostring | string[] - New
getCitationFieldsInputArray()helper method to handle both input formats with deprecation warning - Refactored
getCitationFields()to delegate to the new helper
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `Starting from Atomic v4, the "fields-to-include-in-citations" property will only accept an array of strings. Using a string value is now deprecated. Please update the value to be an array of strings. For example: fields-to-include-in-citations='["fieldA","fieldB"]'` | ||
| ); | ||
| return (this.fieldsToIncludeInCitations ?? '') | ||
| .split(',') |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon after .split(','). This will cause a syntax error.
| .split(',') | |
| .split(','); |
packages/atomic/src/components/search/atomic-generated-answer/atomic-generated-answer.tsx
Show resolved
Hide resolved
packages/atomic/src/components/search/atomic-generated-answer/atomic-generated-answer.tsx
Show resolved
Hide resolved
| } | ||
|
|
||
| // TODO V4 (KIT-5306): Remove support for string value. | ||
| private getCitationFieldsInputArray() { |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should have an explicit return type annotation for type safety. Add : string[] as the return type.
| private getCitationFieldsInputArray() { | |
| private getCitationFieldsInputArray(): string[] { |
| if( Array.isArray(this.fieldsToIncludeInCitations)) { | ||
| return this.fieldsToIncludeInCitations; | ||
| } else { | ||
| this.bindings.engine.logger.warn( | ||
| `Starting from Atomic v4, the "fields-to-include-in-citations" property will only accept an array of strings. Using a string value is now deprecated. Please update the value to be an array of strings. For example: fields-to-include-in-citations='["fieldA","fieldB"]'` | ||
| ); | ||
| return (this.fieldsToIncludeInCitations ?? '') | ||
| .split(',') |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deprecation warning should only be logged when a string value is actually provided. Currently, it will log even when the property is undefined. Consider adding a check:
} else if (this.fieldsToIncludeInCitations) {
this.bindings.engine.logger.warn(
// ... warning message
);
return (this.fieldsToIncludeInCitations ?? '').split(',');
} else {
return [];
}This prevents unnecessary warnings when the property is not set at all.
| if( Array.isArray(this.fieldsToIncludeInCitations)) { | |
| return this.fieldsToIncludeInCitations; | |
| } else { | |
| this.bindings.engine.logger.warn( | |
| `Starting from Atomic v4, the "fields-to-include-in-citations" property will only accept an array of strings. Using a string value is now deprecated. Please update the value to be an array of strings. For example: fields-to-include-in-citations='["fieldA","fieldB"]'` | |
| ); | |
| return (this.fieldsToIncludeInCitations ?? '') | |
| .split(',') | |
| if (Array.isArray(this.fieldsToIncludeInCitations)) { | |
| return this.fieldsToIncludeInCitations; | |
| } else if (typeof this.fieldsToIncludeInCitations === 'string') { | |
| this.bindings.engine.logger.warn( | |
| `Starting from Atomic v4, the "fields-to-include-in-citations" property will only accept an array of strings. Using a string value is now deprecated. Please update the value to be an array of strings. For example: fields-to-include-in-citations='["fieldA","fieldB"]'` | |
| ); | |
| return this.fieldsToIncludeInCitations.split(','); | |
| } else { | |
| return []; |
|
|
||
| // TODO V4 (KIT-5306): Remove support for string value. | ||
| private getCitationFieldsInputArray() { | ||
| if( Array.isArray(this.fieldsToIncludeInCitations)) { |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space after if keyword. Should be if( not if(.
| if( Array.isArray(this.fieldsToIncludeInCitations)) { | |
| if (Array.isArray(this.fieldsToIncludeInCitations)) { |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@louis-bompart I've opened a new pull request, #6739, to work on those changes. Once the pull request is ready, I'll request review from you. |
…6739) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: louis-bompart <[email protected]> Co-authored-by: Louis Bompart <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally I'd say we need to adjust the unit tests accordingly, but given that the component hasn't yet been migrated I guess this will have to wait 😁
lavoiesl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of the a TODO ticket and deprecation warning 👍🏼
| this.bindings.engine.logger.warn( | ||
| `Starting from Atomic v4, the "fields-to-include-in-citations" property will only accept an array of strings. Using a string value is now deprecated. Please update the value to be an array of strings. For example: fields-to-include-in-citations='["fieldA","fieldB"]'` | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR: Should we have a dedicated primitive for raising deprecations?
Here's what Rails does: https://blog.saeloun.com/2024/11/19/rails-7-1-adds-application-deprecators-method/
# 1) Define a Custom Deprecator:
# config/initializers/deprecators.rb
Rails.application.deprecators[:user_full_name] = ActiveSupport::Deprecation.new("2.0", "user_full_name")
# Here, the custom deprecator :user_full_name is defined for deprecations related to the full_name method, specifying the Rails version (2.0) and a name for context.
# 2) Use the Deprecator in Code:
class User < ApplicationRecord
def full_name
Rails.application.deprecators[:user_full_name].warn("The `full_name` method will be removed in the next major release.")
end
endBenefits:
- Encourages defining a target version
- Standardizes how it's reported
- Allows telemetry to track adoption
- Allows silencing those warnings in production use
- Allows deduplication of warnings to avoid spamming in the case of a repeated item
Usually, we use
<my-cmp attr-name="['val1', 'val2']"for attributes. This one differs from the norm, let's fix that :)KIT-5305