feat(ADFA-2994): Increase minimum storage needed for install#987
feat(ADFA-2994): Increase minimum storage needed for install#987dara-abijo-adfa wants to merge 1 commit intostagefrom
Conversation
📝 WalkthroughRelease Notes - ADFA-2994: Increase Minimum Storage Needed for InstallFeature Changes
Technical Changes
Risks & Best Practices Violations
Impact
WalkthroughA feature-flag dependent storage requirement is introduced that replaces a fixed storage threshold with conditional logic. Two new constants define minimum storage for stable and experimental builds, with a computed property selecting between them based on feature flag status. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🤖 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/viewmodel/InstallationViewModel.kt`:
- Around line 42-47: The getter minimumStorageNeeded reads
FeatureFlags.isExperimentsEnabled which indirectly reads the shared flags field
without synchronization; to make it explicitly thread-safe, protect that read by
acquiring the same mutex used in initialize() (use mutex.withLock {
FeatureFlags.isExperimentsEnabled } or wrap the whole conditional in
mutex.withLock) so the computed property cannot observe intermediate flag state,
or alternatively mark the flags field `@Volatile` if you prefer lock-free
visibility—apply the chosen change to minimumStorageNeeded and keep initialize()
unchanged.
| private val minimumStorageNeeded: Long | ||
| get() = if (FeatureFlags.isExperimentsEnabled) { | ||
| MINIMUM_STORAGE_EXPERIMENTAL_GB | ||
| } else { | ||
| MINIMUM_STORAGE_STABLE_GB | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the FeatureFlags file
fd -t f 'FeatureFlags' --extension=ktRepository: appdevforall/CodeOnTheGo
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Search for isExperimentsEnabled definition and usage
rg -n 'isExperimentsEnabled' --type=kotlin -B 2 -A 5Repository: appdevforall/CodeOnTheGo
Length of output: 9661
🏁 Script executed:
#!/bin/bash
# Check for the getStorageInfo method to understand threading context
rg -n 'getStorageInfo' --type=kotlin -B 2 -A 10Repository: appdevforall/CodeOnTheGo
Length of output: 3223
🏁 Script executed:
#!/bin/bash
# Look for FeatureFlags object definition
rg -n 'object FeatureFlags' --type=kotlin -A 30Repository: appdevforall/CodeOnTheGo
Length of output: 3096
🏁 Script executed:
#!/bin/bash
# Check how flags is accessed and if mutex is used
rg -n 'flags\.' common/src/main/java/com/itsaky/androidide/utils/FeatureFlags.kt -B 1 -A 1Repository: appdevforall/CodeOnTheGo
Length of output: 769
🏁 Script executed:
#!/bin/bash
# Check for mutex usage in FeatureFlags
rg -n 'mutex' common/src/main/java/com/itsaky/androidide/utils/FeatureFlags.kt -B 2 -A 2Repository: appdevforall/CodeOnTheGo
Length of output: 367
🏁 Script executed:
#!/bin/bash
# Check the full FeatureFlags file to understand the synchronization strategy
wc -l common/src/main/java/com/itsaky/androidide/utils/FeatureFlags.ktRepository: appdevforall/CodeOnTheGo
Length of output: 136
🏁 Script executed:
#!/bin/bash
# Get the full FeatureFlags file
cat -n common/src/main/java/com/itsaky/androidide/utils/FeatureFlags.ktRepository: appdevforall/CodeOnTheGo
Length of output: 3529
The getter should be protected by the mutex to ensure explicit thread-safety.
The computed property is correct and idiomatic. However, the thread-safety concern is valid: FeatureFlags.isExperimentsEnabled reads flags.experimentsEnabled without acquiring the Mutex, while the initialize() function safely updates flags using mutex.withLock(). This creates an unprotected read path that could see stale or intermediate values during flag updates.
While Boolean field access is atomic at the JVM level (mitigating practical risk), the code is not explicitly thread-safe by design. Either acquire the mutex in the getter or annotate the flags field with @Volatile for clarity and correctness.
🤖 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/viewmodel/InstallationViewModel.kt`
around lines 42 - 47, The getter minimumStorageNeeded reads
FeatureFlags.isExperimentsEnabled which indirectly reads the shared flags field
without synchronization; to make it explicitly thread-safe, protect that read by
acquiring the same mutex used in initialize() (use mutex.withLock {
FeatureFlags.isExperimentsEnabled } or wrap the whole conditional in
mutex.withLock) so the computed property cannot observe intermediate flag state,
or alternatively mark the flags field `@Volatile` if you prefer lock-free
visibility—apply the chosen change to minimumStorageNeeded and keep initialize()
unchanged.
2 participants
Increase the minimum storage needed for installation so users with low storage will be notified.