Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a configuration-driven property data processing system for SkyUI menus. Four ActionScript utilities form a pipeline: ItemFilter validates objects by requirements, PropertyLookup applies keyword and data-member-based property mappings, CompoundProperty generates concatenated composite values, and PropertyDataExtender orchestrates them to process list entries. The system is integrated into the build for six menu targets. ChangesProperty Data Processing Pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@source/actionscript/Common/skyui/props/CompoundProperty.as`:
- Line 17: The assignment this.defaultValues = configObject.defaultValues can
leave undefined values and cause literal "undefined" to be concatenated into
compoundValue and break when configObject.defaultValues or
configObject.concatenateList is missing; update CompoundProperty initialization
and compound generation to defensively check for existence and correct types:
ensure this.defaultValues is set to an empty array if configObject.defaultValues
is falsy or not an Array, ensure this.concatenateList is set to an empty array
or false when missing, and in the compound-building logic (where compoundValue
is assembled) only append values after null/undefined checks (use !== undefined
&& !== null) and skip or substitute defaults instead of concatenating
"undefined", and guard loops with length checks on concatenateList/defaultValues
before indexing.
In `@source/actionscript/Common/skyui/props/PropertyLookup.as`:
- Around line 22-25: The constructor assigns optional maps (defaultValues,
keywords) directly which can be null/undefined and later cause crashes in
processProperty and other methods; normalize these fields to safe defaults
during construction (e.g. set this.defaultValues and this.keywords to empty
objects/arrays when a_configObject.* is falsy) and update any reads in
processProperty, iterateDefaultValues or similar methods to assume non-null maps
(or guard with a short-circuit). Specifically, update the initialization around
propertiesToSet/itemFilter to default this.defaultValues and this.keywords, and
audit processProperty, the loops referenced (the blocks handling
defaultValues/keywords), and any iteration methods to rely on those normalized
fields instead of unguarded access.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c386ab84-4ef1-49fc-aa78-a9e5c0e899ca
📒 Files selected for processing (5)
source/actionscript/Common/skyui/props/CompoundProperty.assource/actionscript/Common/skyui/props/ItemFilter.assource/actionscript/Common/skyui/props/PropertyDataExtender.assource/actionscript/Common/skyui/props/PropertyLookup.assource/swfsources.cmake
| { | ||
| this.propertyToSet = configObject.propertyToSet; | ||
| this.itemFilter = new skyui.props.ItemFilter(configObject.filter); | ||
| this.defaultValues = configObject.defaultValues; |
Contributor
There was a problem hiding this comment.
Prevent "undefined" pollution and null-config crashes in compound generation.
Current initialization can append literal "undefined" to compoundValue, and missing defaultValues/concatenateList can break processing.
💡 Suggested fix
function CompoundProperty(configObject:Object)
{
this.propertyToSet = configObject.propertyToSet;
this.itemFilter = new skyui.props.ItemFilter(configObject.filter);
- this.defaultValues = configObject.defaultValues;
+ this.defaultValues = (configObject.defaultValues instanceof Object) ? configObject.defaultValues : {};
@@
- this.propertyList = configObject.concatenateList;
+ this.propertyList = (configObject.concatenateList instanceof Array) ? configObject.concatenateList : [];
}
@@
- for(var i: Number = 0; i < this.propertyList.length; i++)
+ for(var i: Number = 0; i < this.propertyList.length; i++)
{
- var currProp: String = propertyList[i];
- var currVal: String = String(this.defaultValues[currProp]);
+ var currProp: String = this.propertyList[i];
+ var currVal: String = "";
+ if (this.defaultValues[currProp] != undefined) {
+ currVal = String(this.defaultValues[currProp]);
+ }
if (obj.hasOwnProperty(currProp) && obj[currProp] != undefined && obj[currProp] != "") {
currVal = String(obj[currProp]);
}
compoundValue += this._padString(currVal, this.numPadding, this.padString);
}
- if (compoundValue == "" && this.defaultValues[this.propertyToSet]) {
+ if (compoundValue == "" && this.defaultValues[this.propertyToSet] != undefined) {
compoundValue = this.defaultValues[this.propertyToSet];
}Also applies to: 31-31, 64-66, 73-82
🤖 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 `@source/actionscript/Common/skyui/props/CompoundProperty.as` at line 17, The
assignment this.defaultValues = configObject.defaultValues can leave undefined
values and cause literal "undefined" to be concatenated into compoundValue and
break when configObject.defaultValues or configObject.concatenateList is
missing; update CompoundProperty initialization and compound generation to
defensively check for existence and correct types: ensure this.defaultValues is
set to an empty array if configObject.defaultValues is falsy or not an Array,
ensure this.concatenateList is set to an empty array or false when missing, and
in the compound-building logic (where compoundValue is assembled) only append
values after null/undefined checks (use !== undefined && !== null) and skip or
substitute defaults instead of concatenating "undefined", and guard loops with
length checks on concatenateList/defaultValues before indexing.
Comment on lines
+22
to
+25
| this.propertiesToSet = a_configObject.propertiesToSet; //TODO: eventually determine this automatically? | ||
| this.itemFilter = new skyui.props.ItemFilter(a_configObject.filter); | ||
| this.defaultValues = a_configObject.defaultValues; | ||
| this.keywords = a_configObject.keywords; |
Contributor
There was a problem hiding this comment.
Normalize optional maps before iterating/reading them.
defaultValues and keywords are treated as optional, but processProperty iterates/reads them unguarded. Missing config can crash property processing.
💡 Suggested fix
function PropertyLookup(a_configObject: Object)
{
this.propertiesToSet = a_configObject.propertiesToSet; //TODO: eventually determine this automatically?
this.itemFilter = new skyui.props.ItemFilter(a_configObject.filter);
- this.defaultValues = a_configObject.defaultValues;
- this.keywords = a_configObject.keywords;
+ this.defaultValues = (a_configObject.defaultValues instanceof Object) ? a_configObject.defaultValues : {};
+ this.keywords = (a_configObject.keywords instanceof Object) ? a_configObject.keywords : {};
this._parseDataMemberList(a_configObject);
this.lastValue = -1;
}
@@
- for(var keyword in a_obj.keywords) {
- var keywordValues: Object = this.getKeywordValues(keyword);
- if (keywordValues != undefined) {
- // keyword match found
- for (var propertyToSet in keywordValues) {
- valToSet = keywordValues[propertyToSet];
- // Don't bother setting if default value is already set
- if (valToSet != undefined && valToSet != this.defaultValues[propertyToSet]) {
- a_obj[propertyToSet] = valToSet;
+ if (a_obj.keywords instanceof Object) {
+ for (var keyword in a_obj.keywords) {
+ var keywordValues: Object = this.getKeywordValues(keyword);
+ if (keywordValues != undefined) {
+ // keyword match found
+ for (var propertyToSet in keywordValues) {
+ valToSet = keywordValues[propertyToSet];
+ // Don't bother setting if default value is already set
+ if (valToSet != undefined && valToSet != this.defaultValues[propertyToSet]) {
+ a_obj[propertyToSet] = valToSet;
+ }
}
+ break;
}
- break;
}
}Also applies to: 145-149, 153-167, 181-182
🤖 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 `@source/actionscript/Common/skyui/props/PropertyLookup.as` around lines 22 -
25, The constructor assigns optional maps (defaultValues, keywords) directly
which can be null/undefined and later cause crashes in processProperty and other
methods; normalize these fields to safe defaults during construction (e.g. set
this.defaultValues and this.keywords to empty objects/arrays when
a_configObject.* is falsy) and update any reads in processProperty,
iterateDefaultValues or similar methods to assume non-null maps (or guard with a
short-circuit). Specifically, update the initialization around
propertiesToSet/itemFilter to default this.defaultValues and this.keywords, and
audit processProperty, the loops referenced (the blocks handling
defaultValues/keywords), and any iteration methods to rely on those normalized
fields instead of unguarded access.
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 by CodeRabbit