ADFA-3817 | Fix for dropdown entries separating layout and strings data#1258
Merged
ADFA-3817 | Fix for dropdown entries separating layout and strings data#1258
Conversation
Updates XML generator to extract dropdown items into string arrays and injects them into the project's strings.xml.
jatezzz
force-pushed
the
fix/ADFA-3817-dropdown-entries-experimental
branch
from
eead251 to
705339c
Compare
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (1)
📝 WalkthroughWalkthroughUI designer generation now produces two payloads: layout XML and strings XML. The editor intake asynchronously injects parsed Changes
Sequence DiagramsequenceDiagram
actor User
participant UIDesigner as UIDesignerActivity
participant CV as ComputerVisionActivity
participant Editor as BaseEditorActivity
participant Injector as StringsXmlInjector
participant StringsFile as Project strings.xml
participant Generator as AndroidXmlGenerator
User->>UIDesigner: trigger generation
UIDesigner->>Generator: buildXml()
Generator-->>UIDesigner: (layoutXml, stringsXml)
UIDesigner->>CV: ReturnXmlResult(layoutXml, stringsXml)
CV->>Editor: Intent with RESULT_GENERATED_XML & RESULT_GENERATED_STRINGS + layoutPath
Editor->>Editor: extract extras
alt stringsXml non-empty
Editor->>Injector: inject(layoutPath, stringsXml)
Injector->>StringsFile: update/merge <string-array> entries
StringsFile-->>Injector: success/failure
end
Editor->>Editor: applyGeneratedXmlToEditor(layoutXml)
Editor-->>User: show updated editor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt`:
- Around line 1109-1117: handleStringsInjection currently silently skips
injecting when layoutFilePath is missing and uses hard-coded extra keys; change
it to validate both values with isNullOrBlank (treat empty layoutFilePath as an
error), use named constants instead of literal keys (e.g.,
EXTRA_GENERATED_STRINGS, EXTRA_LAYOUT_FILE_PATH) for Intent extras, and if
layoutFilePath is missing log/report the problem (or surface a user-facing
notification) rather than no-op; when launching editorActivityScope.launch to
call StringsXmlInjector.inject(layoutFilePath, stringsXml) wrap the call in
try/catch and log failures so unresolved `@array/`... references are visible for
debugging.
In `@app/src/main/java/com/itsaky/androidide/utils/StringsXmlInjector.kt`:
- Around line 14-15: The current Regex-based replacement (ARRAY_REGEX) in
StringsXmlInjector should be replaced with proper XML parsing/serialization:
parse the strings.xml via a standard XML parser (e.g.,
javax.xml.parsers.DocumentBuilder / org.w3c.dom or XmlPullParser), locate
<string-array> elements by tag name and name attribute, modify or remove them
programmatically, then write the Document back with an XML serializer to
preserve comments/formatting; update the code paths that reference ARRAY_REGEX
(inside class StringsXmlInjector and any methods that perform the replacement)
to use the new DOM/XmlPull-based logic and remove the regex constant so
commented-out blocks and attribute-order variations are handled correctly.
- Around line 16-28: The inject function currently swallows all exceptions;
change its signature from suspend fun inject(layoutFilePath: String,
newStringsXml: String) to return a Result<Unit> (suspend fun inject(...):
Result<Unit>) and update its body to avoid catching Exception broadly—only catch
and handle specific, expected errors (e.g., IOException when reading/writing the
file and any XML-parsing exception thrown by mergeContent/resolveStringsFile),
returning Result.failure(e) for those cases and Result.success(Unit) on success;
remove the generic catch that logs and absorbs errors (keep logging but still
return failure), and update callers (e.g., BaseEditorActivity) to check the
Result and abort the layout update on failure.
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/viewmodel/ComputerVisionViewModel.kt`:
- Around line 333-338: The code in ComputerVisionViewModel creates a combined
payload but currently discards stringsXml; update the logic that builds combined
(used before calling saveXmlFile) to append stringsXml when it's not blank
(e.g., combine layoutXml, separator/newlines, then stringsXml) so Spinner/arrays
are included in the download payload; make this change in the block that
computes combined and then calls saveXmlFile(combined) and keep the existing
CvAnalyticsUtil and _uiState updates intact.
🪄 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: e9a56a19-b3e6-4138-89ce-b39969940172
📒 Files selected for processing (14)
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.ktapp/src/main/java/com/itsaky/androidide/utils/StringsXmlInjector.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/data/repository/ComputerVisionRepository.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/data/repository/ComputerVisionRepositoryImpl.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/YoloToXmlConverter.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/grammar/AttributeValidator.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/grammar/WidgetGrammar.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/AndroidWidget.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/AndroidXmlGenerator.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/XmlContext.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/ComputerVisionActivity.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/ComputerVisionUiState.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/viewmodel/ComputerVisionViewModel.kt
Collaborator
Author
Current behaviordocument_5154784628711625104.mp4 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/viewmodel/ComputerVisionViewModel.kt (1)
333-335:⚠️ Potential issue | 🟠 Major
stringsXmlis still dropped in the downloads payload.At Line 334,
combinedadds only newlines whenstringsXmlis present, so saved output misses<string-array>data.♻️ Proposed fix
- val combined = if(stringsXml.isNotBlank()) "$layoutXml\n\n\n" else layoutXml + val combined = + if (stringsXml.isNotBlank()) "$layoutXml\n\n\n$stringsXml" else layoutXml🤖 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/ui/viewmodel/ComputerVisionViewModel.kt` around lines 333 - 335, In the onSuccess lambda where layoutXml and stringsXml are combined, the code currently only appends newlines when stringsXml is present, dropping the actual strings payload; fix the logic in the onSuccess block (the combined variable) to append stringsXml (e.g. use layoutXml + separators + stringsXml when stringsXml.isNotBlank()) so the saved/downloaded output includes the <string-array> content, keeping the CvAnalyticsUtil.trackXmlGenerated call and existing state.detections usage intact.
🧹 Nitpick comments (3)
app/src/main/java/com/itsaky/androidide/api/commands/AddStringArrayResourceCommand.kt (3)
65-70: Consider narrowing the exception handling.The catch-all
Exceptionis overly broad. Per project guidelines, prefer catching specific exception types (e.g.,SAXException,IOException,TransformerException) to maintain fail-fast behavior and avoid masking unexpected errors.♻️ Proposed narrower exception handling
- } catch (e: Exception) { + } catch (e: org.xml.sax.SAXException) { + ToolResult.failure( + message = "Failed to parse strings.xml.", + error_details = e.message + ) + } catch (e: java.io.IOException) { + ToolResult.failure( + message = "Failed to read or write strings.xml.", + error_details = e.message + ) + } catch (e: javax.xml.transform.TransformerException) { ToolResult.failure( - message = "An error occurred while adding or updating the string-array resource.", + message = "Failed to serialize strings.xml.", error_details = e.message ) }Based on learnings: "prefer narrow exception handling that catches only the specific exception type reported in crashes... instead of a broad catch-all (e.g., catch (e: Exception))."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/api/commands/AddStringArrayResourceCommand.kt` around lines 65 - 70, The catch-all in AddStringArrayResourceCommand (the catch (e: Exception) that returns ToolResult.failure) should be narrowed: replace it with specific catch clauses for the likely failures such as SAXException, IOException, TransformerException (and ParserConfigurationException if XML parsing is used), each returning ToolResult.failure with the same message and the specific exception's message; leave any truly unexpected errors to propagate (or optionally rethrow/let a higher-level handler catch), ensuring you still pass the exception detail into ToolResult.failure for debugging instead of catching Exception globally.
90-92: Avoid redundantgetElementsByTagNamecalls.
getElementsByTagName("string-array")is called once for.lengthand again for each.item(index). Store theNodeListonce to avoid repeated DOM traversals.♻️ Proposed fix
- val existingNode = List(document.getElementsByTagName("string-array").length) { index -> - document.getElementsByTagName("string-array").item(index) as Element - }.firstOrNull { it.getAttribute("name") == name } + val stringArrayNodes = document.getElementsByTagName("string-array") + val existingNode = (0 until stringArrayNodes.length) + .map { stringArrayNodes.item(it) as Element } + .firstOrNull { it.getAttribute("name") == name }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/api/commands/AddStringArrayResourceCommand.kt` around lines 90 - 92, The code repeatedly calls document.getElementsByTagName("string-array"); cache that NodeList in a local val (e.g., nodeList) in AddStringArrayResourceCommand so you use nodeList.length and nodeList.item(index) instead of calling getElementsByTagName each time, then build the List from that cached nodeList and call .firstOrNull { it.getAttribute("name") == name } as before.
33-36:fileMutexescan grow unbounded.The
ConcurrentHashMapaccumulates entries for every unique file path but never removes them. In long-running sessions with many differentstrings.xmlpaths (e.g., multi-module projects), this could cause memory to grow without bound.Consider using a bounded cache (e.g.,
CacheBuilderwith weak values or size limit) or cleaning up entries when they're no longer needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/utils/ProjectStringsXmlResolver.kt`:
- Around line 21-23: The code currently returns stringsFile.takeIf { it.exists()
&& it.isFile } but doesn't ensure the canonicalized stringsFile is contained
within the project root (symlinks can escape the project). After computing val
stringsFile = File(projectRoot, STRINGS_XML_RELATIVE_PATH).canonicalFile, also
canonicalize the project root (e.g., val canonicalProjectRoot =
projectRoot.canonicalFile) and add a containment check (e.g.,
stringsFile.path.startsWith(canonicalProjectRoot.path + File.separator) or use a
proper Path relativize/startsWith) to the takeIf predicate so you only return
the file when it exists, isFile, and is located inside canonicalProjectRoot;
keep referencing stringsFile, projectRoot and STRINGS_XML_RELATIVE_PATH when
making the change.
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/AndroidWidget.kt`:
- Around line 89-104: The code in processAttributes (AttributeKey.ENTRIES
handling) builds an array name from the raw id which may contain characters
invalid for Android resources and splits on commas naive to commas inside items;
fix by sanitizing the id into a valid Android resource name (lowercase, replace
any non [a-z0-9_] with '_', ensure it starts with a letter (prefix like "a_" if
needed), and fall back to nextId() or a safe suffix if result is empty) when
forming arrayName used with context.stringArrays and processed[key], and replace
the simple value.split(",") with a CSV-safe split (respect quoted items) or a
small parser that trims and unquotes items so commas inside quoted entries are
preserved before storing items in stringArrays and setting processed[key] =
"@array/$arrayName".
---
Duplicate comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/viewmodel/ComputerVisionViewModel.kt`:
- Around line 333-335: In the onSuccess lambda where layoutXml and stringsXml
are combined, the code currently only appends newlines when stringsXml is
present, dropping the actual strings payload; fix the logic in the onSuccess
block (the combined variable) to append stringsXml (e.g. use layoutXml +
separators + stringsXml when stringsXml.isNotBlank()) so the saved/downloaded
output includes the <string-array> content, keeping the
CvAnalyticsUtil.trackXmlGenerated call and existing state.detections usage
intact.
---
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/api/commands/AddStringArrayResourceCommand.kt`:
- Around line 65-70: The catch-all in AddStringArrayResourceCommand (the catch
(e: Exception) that returns ToolResult.failure) should be narrowed: replace it
with specific catch clauses for the likely failures such as SAXException,
IOException, TransformerException (and ParserConfigurationException if XML
parsing is used), each returning ToolResult.failure with the same message and
the specific exception's message; leave any truly unexpected errors to propagate
(or optionally rethrow/let a higher-level handler catch), ensuring you still
pass the exception detail into ToolResult.failure for debugging instead of
catching Exception globally.
- Around line 90-92: The code repeatedly calls
document.getElementsByTagName("string-array"); cache that NodeList in a local
val (e.g., nodeList) in AddStringArrayResourceCommand so you use nodeList.length
and nodeList.item(index) instead of calling getElementsByTagName each time, then
build the List from that cached nodeList and call .firstOrNull {
it.getAttribute("name") == name } as before.
🪄 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: 0e1fa2e4-d685-4dae-ac80-6beaed3a763f
📒 Files selected for processing (20)
app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.ktapp/src/main/java/com/itsaky/androidide/api/commands/AddStringArrayResourceCommand.ktapp/src/main/java/com/itsaky/androidide/api/commands/AddStringResourceCommand.ktapp/src/main/java/com/itsaky/androidide/api/commands/SuspendCommand.ktapp/src/main/java/com/itsaky/androidide/utils/ProjectStringsXmlResolver.ktapp/src/main/java/com/itsaky/androidide/utils/StringsXmlInjector.ktapp/src/main/res/values/strings.xmlcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/data/repository/ComputerVisionRepository.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/data/repository/ComputerVisionRepositoryImpl.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/YoloToXmlConverter.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/grammar/AttributeValidator.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/grammar/WidgetGrammar.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/AndroidWidget.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/AndroidXmlGenerator.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/XmlContext.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/ComputerVisionActivity.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/ComputerVisionUiState.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/viewmodel/ComputerVisionViewModel.ktuidesigner/src/main/java/com/itsaky/androidide/uidesigner/UIDesignerActivity.kt
✅ Files skipped from review due to trivial changes (3)
- cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/XmlContext.kt
- uidesigner/src/main/java/com/itsaky/androidide/uidesigner/UIDesignerActivity.kt
- app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (7)
- cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/data/repository/ComputerVisionRepository.kt
- cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/ComputerVisionUiState.kt
- cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/FuzzyAttributeParser.kt
- cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/YoloToXmlConverter.kt
- cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/grammar/WidgetGrammar.kt
- cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/grammar/AttributeValidator.kt
- app/src/main/java/com/itsaky/androidide/utils/StringsXmlInjector.kt
Daniel-ADFA
approved these changes
Apr 28, 2026
Contributor
Daniel-ADFA
left a comment
Daniel-ADFA
left a comment
There was a problem hiding this comment.
Approving with clarifying questions
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.
Description
Resolved an issue where dropdowns incorrectly generated
tools:entries="[x,y,z]"inline, which fails to compile. Extracted the dropdown array definitions so that the layout is properly separated from the data. The generator now creates<string-array>items and assigns standard@array/...references to the Spinner'sandroid:entriesattribute.Details
AndroidXmlGeneratorand the Computer Vision pipeline to return aPair<String, String>containing both the layout XML and the newly generated strings XML.SpinnerWidgetparsing logic to cleanly format array items and populateXmlContext.stringArrays.StringsXmlInjectorutility to safely inject the generated<string-array>blocks into the project's existingres/values/strings.xmlwithout destroying current resources.BaseEditorActivityto handle the dual XML results and trigger the injection appropriately.document_5152532828897937754.mp4
Ticket
ADFA-3817
Observation
The
StringsXmlInjectoruses regex to check for existing arrays with the same name to prevent duplication, and cleanly inserts new blocks right before the closing</resources>tag.