ADFA-3492 | Fix ProjectFile NPE in resource fragments#1341
Open
jatezzz wants to merge 1 commit into
Open
Conversation
…agments Introduces ProjectResolver to safely fetch project instances and handle null states
jatezzz
requested review from
a team,
Daniel-ADFA,
dara-abijo-adfa and
itsaky-adfa
Contributor
📝 WalkthroughRelease NotesBug Fixes
ChangesProject Resolution Pattern
Fragment Updates
ResourceManagerActivity Refactoring
Risk Assessment & Best Practices✓ Positive Patterns
⚠ Risks & Considerations
Code Quality Metrics
WalkthroughThis PR refactors project resolution across resource management components. It introduces a ChangesResource Fragment and Activity Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@layouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/activities/ResourceManagerActivity.kt`:
- Around line 63-65: The current logic uses
ProjectManager.instance.openedProject as primary and falls back to the intent,
which can ignore an explicitly requested project; change the assignment so
project is set from the IntentCompat.getParcelableExtra(...) result
(projectExtra) first, and then update/sync ProjectManager.instance.openedProject
to that project (or call ProjectManager.instance.openProject/openedProject
setter) so both the activity and ProjectManager reference the intent project;
apply the same change to the other occurrence(s) that currently use
openedProject ?: projectExtra (lines around the ProjectManager usage).
- Around line 205-207: The case for option 1 only calls launchPhotoPicker()
behind an API >= TIRAMISU check, so on API < 33 the option does nothing; update
the branch to handle legacy devices by invoking a fallback picker for older APIs
(e.g., call launchPhotoPicker() for API >= Build.VERSION_CODES.TIRAMISU and call
a new or existing legacy method such as launchPhotoPickerLegacy() or
startActivityForResult/Intent ACTION_PICK / ACTION_OPEN_DOCUMENT targeting
MediaStore.Images.Media.EXTERNAL_CONTENT_URI for API < TIRAMISU). Modify the
ResourceManagerActivity's selection handling (the switch/case that currently
references launchPhotoPicker()) and add the legacy picker helper (or reuse an
existing gallery/open-document helper) so image drawable selection works on
pre-Android 13 devices.
In
`@layouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/fragments/resources/FontFragment.java`:
- Around line 71-73: onViewCreated may return early when
ProjectResolver.getValidProjectOrShowError(...) yields null leaving the field
executor uninitialized, so update onDestroyView to guard the teardown: check
that executor is non-null (and optionally not already shutdown/terminated)
before calling executor.shutdownNow(), and set executor to null after shutdown;
reference the executor field and the lifecycle methods
onViewCreated/onDestroyView and the ProjectResolver.getValidProjectOrShowError
call when making the change.
In
`@layouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/utils/ProjectResolver.java`:
- Around line 15-18: resolveProject() currently prefers the singleton
ProjectManager.getInstance().getOpenedProject() which can ignore an explicitly
provided fragment argument; change the lookup order to first check
arguments.getParcelable(Constants.EXTRA_KEY_PROJECT) (or equivalent fragment
arguments retrieval) and return it if non-null, otherwise fall back to
ProjectManager.getInstance().getOpenedProject(); ensure null-safe handling of
arguments and the parcelable cast to avoid NPEs.
🪄 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: f12afb07-7bb8-4223-b21c-0ce41d23d2f8
📒 Files selected for processing (6)
layouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/activities/ResourceManagerActivity.ktlayouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/fragments/resources/ColorFragment.javalayouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/fragments/resources/DrawableFragment.ktlayouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/fragments/resources/FontFragment.javalayouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/fragments/resources/StringFragment.javalayouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/utils/ProjectResolver.java
Comment on lines
+63
to
+65
| val projectExtra = IntentCompat.getParcelableExtra(intent, Constants.EXTRA_KEY_PROJECT, ProjectFile::class.java) | ||
| project = ProjectManager.instance.openedProject ?: projectExtra | ||
|
|
Contributor
There was a problem hiding this comment.
Use the intent project as the primary source and sync ProjectManager accordingly.
The current precedence (openedProject ?: projectExtra) can ignore the explicitly requested project and show/edit the wrong project.
💡 Proposed fix
val projectExtra = IntentCompat.getParcelableExtra(intent, Constants.EXTRA_KEY_PROJECT, ProjectFile::class.java)
-project = ProjectManager.instance.openedProject ?: projectExtra
+project = projectExtra ?: ProjectManager.instance.openedProject
if (project == null) {
finish()
return@launch
}
-if (ProjectManager.instance.openedProject == null) {
- ProjectManager.instance.openProject(project)
+project?.let { resolvedProject ->
+ if (ProjectManager.instance.openedProject?.path != resolvedProject.path) {
+ ProjectManager.instance.openProject(resolvedProject)
+ }
}Also applies to: 71-73
🤖 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
`@layouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/activities/ResourceManagerActivity.kt`
around lines 63 - 65, The current logic uses
ProjectManager.instance.openedProject as primary and falls back to the intent,
which can ignore an explicitly requested project; change the assignment so
project is set from the IntentCompat.getParcelableExtra(...) result
(projectExtra) first, and then update/sync ProjectManager.instance.openedProject
to that project (or call ProjectManager.instance.openProject/openedProject
setter) so both the activity and ProjectManager reference the intent project;
apply the same change to the other occurrence(s) that currently use
openedProject ?: projectExtra (lines around the ProjectManager usage).
Comment on lines
+205
to
+207
| 1 -> if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { | ||
| launchPhotoPicker() | ||
| } |
Contributor
There was a problem hiding this comment.
Image Drawable selection is a no-op on pre-Android 13 devices.
On API < 33, option 1 does nothing, so users cannot add image drawables from this picker.
💡 Proposed fix
-1 -> if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
- launchPhotoPicker()
-}
+1 -> {
+ if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
+ launchPhotoPicker()
+ } else {
+ photoPicker?.launch("image/*")
+ }
+}🤖 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
`@layouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/activities/ResourceManagerActivity.kt`
around lines 205 - 207, The case for option 1 only calls launchPhotoPicker()
behind an API >= TIRAMISU check, so on API < 33 the option does nothing; update
the branch to handle legacy devices by invoking a fallback picker for older APIs
(e.g., call launchPhotoPicker() for API >= Build.VERSION_CODES.TIRAMISU and call
a new or existing legacy method such as launchPhotoPickerLegacy() or
startActivityForResult/Intent ACTION_PICK / ACTION_OPEN_DOCUMENT targeting
MediaStore.Images.Media.EXTERNAL_CONTENT_URI for API < TIRAMISU). Modify the
ResourceManagerActivity's selection handling (the switch/case that currently
references launchPhotoPicker()) and add the legacy picker helper (or reuse an
existing gallery/open-document helper) so image drawable selection works on
pre-Android 13 devices.
Comment on lines
+71
to
+73
| project = ProjectResolver.getValidProjectOrShowError(getArguments(), view); | ||
| if (project == null) return; | ||
|
|
Contributor
There was a problem hiding this comment.
Guard executor teardown when project resolution fails.
If project is null, onViewCreated returns before executor is initialized; onDestroyView() then calls shutdownNow() on null and can crash.
💡 Proposed fix
`@Override`
public void onDestroyView() {
super.onDestroyView();
- executor.shutdownNow();
+ if (executor != null) {
+ executor.shutdownNow();
+ executor = null;
+ }
binding = null;
}🤖 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
`@layouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/fragments/resources/FontFragment.java`
around lines 71 - 73, onViewCreated may return early when
ProjectResolver.getValidProjectOrShowError(...) yields null leaving the field
executor uninitialized, so update onDestroyView to guard the teardown: check
that executor is non-null (and optionally not already shutdown/terminated)
before calling executor.shutdownNow(), and set executor to null after shutdown;
reference the executor field and the lifecycle methods
onViewCreated/onDestroyView and the ProjectResolver.getValidProjectOrShowError
call when making the change.
Comment on lines
+15
to
+18
| ProjectFile openedProject = ProjectManager.getInstance().getOpenedProject(); | ||
|
|
||
| if (openedProject != null) return openedProject; | ||
| if (arguments != null) return arguments.getParcelable(Constants.EXTRA_KEY_PROJECT); |
Contributor
There was a problem hiding this comment.
Prefer fragment arguments over singleton state in project resolution.
resolveProject() currently returns openedProject first, which can ignore the explicitly injected fragment argument and resolve the wrong project.
💡 Proposed fix
public static ProjectFile resolveProject(`@Nullable` Bundle arguments) {
- ProjectFile openedProject = ProjectManager.getInstance().getOpenedProject();
-
- if (openedProject != null) return openedProject;
- if (arguments != null) return arguments.getParcelable(Constants.EXTRA_KEY_PROJECT);
-
- return null;
+ if (arguments != null) {
+ ProjectFile fromArgs = arguments.getParcelable(Constants.EXTRA_KEY_PROJECT);
+ if (fromArgs != null) return fromArgs;
+ }
+ return ProjectManager.getInstance().getOpenedProject();
}🤖 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
`@layouteditor/src/main/java/org/appdevforall/codeonthego/layouteditor/utils/ProjectResolver.java`
around lines 15 - 18, resolveProject() currently prefers the singleton
ProjectManager.getInstance().getOpenedProject() which can ignore an explicitly
provided fragment argument; change the lookup order to first check
arguments.getParcelable(Constants.EXTRA_KEY_PROJECT) (or equivalent fragment
arguments retrieval) and return it if non-null, otherwise fall back to
ProjectManager.getInstance().getOpenedProject(); ensure null-safe handling of
arguments and the parcelable cast to avoid NPEs.
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.
Description
Resolves a
NullPointerExceptionoccurring inColorFragment(and other resource fragments) when attempting to accessProjectFilemethods likegetColorsPath()on a null object reference. This PR introduces aProjectResolverto safely resolve the project instance fromProjectManageror the fragment arguments. Additionally, it transitions the resource fragments to use thenewInstancepattern, ensuring theProjectFileis securely passed and retained across the fragment's lifecycle.Details
ProjectResolver.javato handle safe project data extraction and display graceful error states if the project is null.ColorFragment,DrawableFragment,FontFragment, andStringFragmentto use anewInstance(ProjectFile)method instead of relying solely on the singleton or non-default constructors.ResourceManagerActivityto handle project intent loading correctly usingIntentCompatand cleaned up its options menu action logic.Before
Screen.Recording.2026-05-26.at.3.07.19.PM.mov
After
Screen.Recording.2026-05-26.at.3.40.42.PM.mov
Ticket
ADFA-3492