ADFA-3782 | Implement UI grammar for CV module#1220
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 19 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughFuzzyAttributeParser now delegates categorical validation to a new UiGrammarValidator which enforces widget-specific allowlists and fuzzy-matches categorical attribute values; AndroidXmlGenerator no longer injects default Changes
Sequence Diagram(s)sequenceDiagram
participant OCR as OCR / CV extractor
participant Parser as FuzzyAttributeParser
participant Validator as UiGrammarValidator
participant Generator as AndroidXmlGenerator
OCR->>Parser: produce raw attribute map + tag
Parser->>Validator: enforceGrammar(rawParsedAttributes, tag)
Validator-->>Parser: filtered/normalized attributes
Parser->>Generator: pass validated attributes
Generator-->>Parser: produce XML element (no forced image defaults)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/FuzzyAttributeParser.kt (1)
164-176:⚠️ Potential issue | 🟠 MajorGrammar enforcement collapses all unlisted widgets to empty maps.
enforceGrammar()returnsemptyMap()for any widget not inallowedAttributesByWidget. Looking at the current allowlist, only 8 widget types are enumerated: Spinner, ImageView, EditText, Button, CheckBox, RadioButton, Slider (qualified), and Switch. This means calls toparse()with TextView, View, LinearLayout, ProgressBar, CardView, and other unlisted widgets will now return empty maps regardless of successful parsing.The test file contains 40+ test cases calling
parse()with unlisted widgets (e.g., TextView, View, LinearLayout). These tests will fail after this change. Either expandallowedAttributesByWidgetto include all tested widget types, or update tests to reflect the new fail-closed semantics. Resolve the enumeration strategy inUiGrammarValidator.ktbefore merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/FuzzyAttributeParser.kt` around lines 164 - 176, enforceGrammar currently returns emptyMap() for any widget not present in allowedAttributesByWidget, causing parse() calls for unlisted widgets (e.g., TextView, View, LinearLayout) to lose parsed attributes; fix by updating UiGrammarValidator.enforceGrammar to treat unknown widgets permissively (i.e., if allowedAttributesByWidget does not contain the tag, return the raw attributes map instead of emptyMap()), or alternatively expand the allowedAttributesByWidget set in UiGrammarValidator to include all widget types exercised by the tests (ensuring parse() and enforceGrammar remain consistent).
🧹 Nitpick comments (3)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt (3)
19-19:inputTypeValuescovers only a small subset of Android input types.Values such as
textMultiLine,textCapSentences,textAutoComplete,datetime,date,time,textVisiblePassword, andnumberSignedare valid and common. SincematchCategoricalValueuses a 70% fuzzy threshold, inputs that are close to a listed value but conceptually different (e.g.,textMultiLine→ fuzzy match totext) may be silently coerced to the wrong value rather than preserved. If the PR's intent is to restrict to the spreadsheet's enumerated set, that's fine; otherwise expand the list and/or document this narrowing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt` at line 19, The inputTypeValues constant in UiGrammarValidator.kt is too narrow and can cause matchCategoricalValue to fuzzy-match or coerce valid Android input types incorrectly; expand inputTypeValues to include the common Android types mentioned (e.g., textMultiLine, textCapSentences, textAutoComplete, datetime, date, time, textVisiblePassword, numberSigned, etc.) or document that the validator intentionally restricts to a curated whitelist; update the list used by inputTypeValues and ensure matchCategoricalValue still uses the intended 70% threshold (or adjust threshold) so valid types are preserved rather than silently coerced.
6-15: Consider sourcing the allowlist from the spreadsheet rather than duplicating it in code.The PR description states the grammar comes from a metadata spreadsheet. Hardcoding the same mappings in a
.ktfile risks drift when the spreadsheet is updated. If the spreadsheet is already part of the repo (or gets generated as a resource), consider loading this map from it — or at minimum, add a comment referencing the spreadsheet/ticket and stating that any change must be mirrored there. Optional, given the PR is marked experimental.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt` around lines 6 - 15, The hardcoded allowlist map allowedAttributesByWidget in UiGrammarValidator.kt should not drift from the source spreadsheet; update the code to load this mapping from the canonical metadata (e.g., parse the repository spreadsheet or a generated resource at startup) and replace the inline map with a loader function (e.g., loadAllowedAttributesFromMetadata()) that returns the same Map<String, Set<String>; if you cannot implement loading now, at minimum add a clear TODO comment above allowedAttributesByWidget pointing to the exact spreadsheet/ticket and stating that any changes must be mirrored there and mark the data as experimental so reviewers know it’s temporary.
38-51: Dimension suffix check is too narrow.
enforceValueGrammaronly acceptsdp/sp/pxas valid dimension units (Line 42). Android supportsdip,sip,pt,in,mmas well. With the upstreamcleanDimensiontypically normalizing todp/sp, this rarely fires in practice, but any value that survives as e.g.16ptwill be dropped into the fuzzy matcher againstdimensionValues(match_parent,wrap_content) and be discarded when it doesn't cross the 70% threshold. Consider accepting any recognized Android dimension suffix, or a regex likeRegex("^-?\\d+(\\.\\d+)?(dp|dip|sp|sip|px|pt|in|mm)$"), before falling back to categorical matching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt` around lines 38 - 51, The current enforceValueGrammar function only accepts "dp"/"sp"/"px" which drops valid Android dimensions; update enforceValueGrammar to recognize all Android dimension suffixes (e.g., dp|dip|sp|sip|px|pt|in|mm) by applying a regex check (e.g., Regex("^-?\\d+(\\.\\d+)?(dp|dip|sp|sip|px|pt|in|mm)\$")) to trimmed before falling back to matchCategoricalValue, so numeric dimensions like "16pt" or "8dip" are returned intact; adjust any logic around matchCategoricalValue and dimensionValues to only be used when the regex does not match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt`:
- Around line 6-36: enforceGrammar currently drops all attributes for tags not
present in allowedAttributesByWidget; change it to "fail open" by, when
allowedAttributesByWidget[tag] is null, iterating rawParsedAttributes and
running enforceValueGrammar on each entry and returning that filtered map
instead of emptyMap(); also update allowedAttributesByWidget entry for "Spinner"
to include "android:entries" (or include both "tools:entries" and
"android:entries") so runtime spinners keep their entries; touch enforceGrammar
and allowedAttributesByWidget to implement these two changes.
---
Outside diff comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/FuzzyAttributeParser.kt`:
- Around line 164-176: enforceGrammar currently returns emptyMap() for any
widget not present in allowedAttributesByWidget, causing parse() calls for
unlisted widgets (e.g., TextView, View, LinearLayout) to lose parsed attributes;
fix by updating UiGrammarValidator.enforceGrammar to treat unknown widgets
permissively (i.e., if allowedAttributesByWidget does not contain the tag,
return the raw attributes map instead of emptyMap()), or alternatively expand
the allowedAttributesByWidget set in UiGrammarValidator to include all widget
types exercised by the tests (ensuring parse() and enforceGrammar remain
consistent).
---
Nitpick comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt`:
- Line 19: The inputTypeValues constant in UiGrammarValidator.kt is too narrow
and can cause matchCategoricalValue to fuzzy-match or coerce valid Android input
types incorrectly; expand inputTypeValues to include the common Android types
mentioned (e.g., textMultiLine, textCapSentences, textAutoComplete, datetime,
date, time, textVisiblePassword, numberSigned, etc.) or document that the
validator intentionally restricts to a curated whitelist; update the list used
by inputTypeValues and ensure matchCategoricalValue still uses the intended 70%
threshold (or adjust threshold) so valid types are preserved rather than
silently coerced.
- Around line 6-15: The hardcoded allowlist map allowedAttributesByWidget in
UiGrammarValidator.kt should not drift from the source spreadsheet; update the
code to load this mapping from the canonical metadata (e.g., parse the
repository spreadsheet or a generated resource at startup) and replace the
inline map with a loader function (e.g., loadAllowedAttributesFromMetadata())
that returns the same Map<String, Set<String>; if you cannot implement loading
now, at minimum add a clear TODO comment above allowedAttributesByWidget
pointing to the exact spreadsheet/ticket and stating that any changes must be
mirrored there and mark the data as experimental so reviewers know it’s
temporary.
- Around line 38-51: The current enforceValueGrammar function only accepts
"dp"/"sp"/"px" which drops valid Android dimensions; update enforceValueGrammar
to recognize all Android dimension suffixes (e.g., dp|dip|sp|sip|px|pt|in|mm) by
applying a regex check (e.g.,
Regex("^-?\\d+(\\.\\d+)?(dp|dip|sp|sip|px|pt|in|mm)\$")) to trimmed before
falling back to matchCategoricalValue, so numeric dimensions like "16pt" or
"8dip" are returned intact; adjust any logic around matchCategoricalValue and
dimensionValues to only be used when the regex does not match.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 233a9f2e-45c0-404e-afa7-9b6ff1c63fb0
📒 Files selected for processing (2)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/FuzzyAttributeParser.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt
jatezzz
force-pushed
the
feat/ADFA-3782-ui-grammar-spreadsheet-rules-experimental
branch
from
8f3fcbf to
4a93144
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/FuzzyAttributeParser.kt (1)
33-33:⚠️ Potential issue | 🟠 MajorResolve the ambiguous
stylealias before enforcing Slider grammar.A raw
stylekey matchesTEXT_STYLEfirst, so Line 175 filters it asandroid:textStyle; for Slider, the allowlist expectsstyle, so valid Slider style annotations are dropped.🐛 Minimal parser-side fix
- TEXT_STYLE("android:textStyle", listOf("textstyle", "text_style", "style"), ValueType.RAW), + TEXT_STYLE("android:textStyle", listOf("textstyle", "text_style"), ValueType.RAW),Also applies to: 88-89, 175-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/FuzzyAttributeParser.kt` at line 33, The alias "style" in the TEXT_STYLE enum entry (TEXT_STYLE("android:textStyle", listOf("textstyle", "text_style", "style"), ValueType.RAW)) is ambiguous and causes parser code that normalizes keys to map plain "style" to "android:textStyle", which then prevents Slider grammar (which expects a raw "style" key) from matching; fix by removing "style" from TEXT_STYLE's alias list (or otherwise guarding that alias in the attribute-normalization logic) so that plain "style" remains available for Slider grammar matching—update the TEXT_STYLE declaration and any normalization function that uses ValueType.RAW to ensure "style" is not auto-mapped to android:textStyle.
♻️ Duplicate comments (2)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt (2)
7-7:⚠️ Potential issue | 🟠 MajorAllow the Spinner entries key the parser actually emits.
Line 7 keeps
android:entries, butFuzzyAttributeParser.AttributeKey.ENTRIEScurrently emitstools:entries; afterenforceGrammar, parsed Spinner entries are still dropped unless the parser is changed too.🐛 Minimal allowlist fix
- "Spinner" to setOf("android:layout_width", "android:layout_height", "android:id", "android:text", "android:entries"), + "Spinner" to setOf("android:layout_width", "android:layout_height", "android:id", "android:text", "android:entries", "tools:entries"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt` at line 7, The Spinner allowlist in UiGrammarValidator (the map entry for "Spinner") is using "android:entries" while the parser emits FuzzyAttributeParser.AttributeKey.ENTRIES as "tools:entries", so parsed Spinner entries get dropped by enforceGrammar; update the Spinner entry to allow the parser output by including "tools:entries" (or both "android:entries" and "tools:entries") so FuzzyAttributeParser.AttributeKey.ENTRIES is accepted for Spinner during validation.
23-36:⚠️ Potential issue | 🟠 MajorApply value grammar even when failing open.
Line 24 returns raw attributes for unlisted tags, so
TextView,View,CardView, and other fail-open tags bypass the categorical normalization that was moved out ofcleanValue.🐛 Preserve fail-open keys while still validating values
fun enforceGrammar(rawParsedAttributes: Map<String, String>, tag: String): Map<String, String> { - val allowedKeys = allowedAttributesByWidget[tag] ?: return rawParsedAttributes - + val allowedKeys = allowedAttributesByWidget[tag] val filteredMap = mutableMapOf<String, String>() for ((key, rawValue) in rawParsedAttributes) { - if (key in allowedKeys) { + if (allowedKeys == null || key in allowedKeys) { val validValue = enforceValueGrammar(key, rawValue) if (validValue != null) { filteredMap[key] = validValue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt` around lines 23 - 36, The current enforceGrammar returns rawParsedAttributes immediately when tag is not found in allowedAttributesByWidget, causing fail-open tags (e.g., TextView/View/CardView) to skip value normalization; change enforceGrammar so that when allowedAttributesByWidget[tag] is null it still iterates rawParsedAttributes and applies enforceValueGrammar to each (preserving keys whose enforceValueGrammar returns non-null) while for known tags keep filtering by allowedKeys—use the existing loop and enforceValueGrammar call rather than early returning rawParsedAttributes to ensure values are validated even for unlisted tags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt`:
- Line 21: The validator currently emits bare category names from sliderStyles
(e.g., "continuous", "material") directly into XML style attributes, which
produces invalid XML; update the code in UiGrammarValidator (where sliderStyles
is used) to map these category keys to concrete XML style tokens (for example, a
lookup that converts "material" -> "@style/Widget.MaterialComponents.Slider" or
to a theme attribute like "?attr/sliderStyle") and emit that mapped value into
the style attribute instead of the raw category name; ensure the mapping covers
all entries in sliderStyles and add a fallback/validation error path if an
unknown category is encountered.
---
Outside diff comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/FuzzyAttributeParser.kt`:
- Line 33: The alias "style" in the TEXT_STYLE enum entry
(TEXT_STYLE("android:textStyle", listOf("textstyle", "text_style", "style"),
ValueType.RAW)) is ambiguous and causes parser code that normalizes keys to map
plain "style" to "android:textStyle", which then prevents Slider grammar (which
expects a raw "style" key) from matching; fix by removing "style" from
TEXT_STYLE's alias list (or otherwise guarding that alias in the
attribute-normalization logic) so that plain "style" remains available for
Slider grammar matching—update the TEXT_STYLE declaration and any normalization
function that uses ValueType.RAW to ensure "style" is not auto-mapped to
android:textStyle.
---
Duplicate comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt`:
- Line 7: The Spinner allowlist in UiGrammarValidator (the map entry for
"Spinner") is using "android:entries" while the parser emits
FuzzyAttributeParser.AttributeKey.ENTRIES as "tools:entries", so parsed Spinner
entries get dropped by enforceGrammar; update the Spinner entry to allow the
parser output by including "tools:entries" (or both "android:entries" and
"tools:entries") so FuzzyAttributeParser.AttributeKey.ENTRIES is accepted for
Spinner during validation.
- Around line 23-36: The current enforceGrammar returns rawParsedAttributes
immediately when tag is not found in allowedAttributesByWidget, causing
fail-open tags (e.g., TextView/View/CardView) to skip value normalization;
change enforceGrammar so that when allowedAttributesByWidget[tag] is null it
still iterates rawParsedAttributes and applies enforceValueGrammar to each
(preserving keys whose enforceValueGrammar returns non-null) while for known
tags keep filtering by allowedKeys—use the existing loop and enforceValueGrammar
call rather than early returning rawParsedAttributes to ensure values are
validated even for unlisted tags.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6472b1e2-1c3b-4cd0-a6a1-6d5b51823789
📒 Files selected for processing (3)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/AndroidXmlGenerator.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/FuzzyAttributeParser.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/UiGrammarValidator.kt
💤 Files with no reviewable changes (1)
- cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/AndroidXmlGenerator.kt
jatezzz
force-pushed
the
feat/ADFA-3782-ui-grammar-spreadsheet-rules-experimental
branch
from
4a93144 to
ac84b39
Compare
jatezzz
force-pushed
the
feat/ADFA-3782-ui-grammar-spreadsheet-rules-experimental
branch
2 times, most recently
from
4a96aca to
46a0b65
Compare
Extracted `UiGrammarValidator` to strictly enforce widget-to-attribute mappings and allowed values. `FuzzyAttributeParser` has been cleaned up to only handle OCR tokenization and delegation.
… spreadsheet, added a validation to the allowed attributes detection and to the slider styles
jatezzz
force-pushed
the
feat/ADFA-3782-ui-grammar-spreadsheet-rules-experimental
branch
from
46a0b65 to
c84fde9
Compare
jatezzz
deleted the
feat/ADFA-3782-ui-grammar-spreadsheet-rules-experimental
branch
2 participants
Description
This PR implements the UI grammar for the CV module as specified in the metadata mappings. It introduces a validation layer that ensures extracted attributes match the allowed properties for each specific widget type and enforces valid values for categorical attributes like layout dimensions, gravity, and input types.
Details
Logic-related:
UiGrammarValidator.ktto centralize attribute filtering logic based on widget type (e.g., Spinner, ImageView, EditText).enforceGrammarinto the parsing workflow withinFuzzyAttributeParser.kt.layout_gravity,inputType,textStyle, andstyleusing fuzzy matching to improve OCR reliability.document_5129774364132116196.mp4
Ticket
ADFA-3782