Refactor gradle build protocols and fix time interval bug for overnight hours of eateries#147
Open
RyanCheung555 wants to merge 4 commits intomainfrom
Open
Refactor gradle build protocols and fix time interval bug for overnight hours of eateries#147RyanCheung555 wants to merge 4 commits intomainfrom
RyanCheung555 wants to merge 4 commits intomainfrom
Conversation
…ies depending on build version. update gitignore.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR replaces Gradle secrets-plugin with explicit properties loading, migrates annotation processing from KAPT to KSP, updates top-level plugins (Kotlin/Compose/KSP/Apollo), adds signing/App Bundle handling and .gitignore entries, disables AndroidX Profile Installer in the manifest, and refactors TimeUtils to handle overnight intervals via LocalDateTime. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 3
🧹 Nitpick comments (2)
app/build.gradle.kts (2)
80-117: Reduce duplicated build-type URL wiring to avoid config drift.
BACKEND_URL/EATERY_URL/UPLIFT_URLsetup is repeated in debug, ecosystem, and release. Consider a small helper that takes a prefix (DEBUG/PROD) and applies all related fields/placeholders once.Also applies to: 132-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/build.gradle.kts` around lines 80 - 117, The debug/ecosystem/release blocks duplicate wiring of BACKEND_URL/EATERY_URL/UPLIFT_URL and manifestPlaceholders; extract a helper function (e.g., configureUrlsForVariant(prefix: String, target: com.android.build.gradle.internal.dsl.BuildType) or an extension on DefaultConfig/BuildType) that calls buildConfigField("String", "BACKEND_URL", asBuildConfigString(secretsProperties.requireString("${prefix}_BACKEND_URL"))), manifestPlaceholders["BACKEND_URL"] = asManifestPlaceholder(...), and similar for EATERY_URL and UPLIFT_URL, then call that helper from the debug, create("ecosystem") and release blocks (keep ECOSYSTEM_FLAG where needed) to remove the repeated buildConfigField/manifestPlaceholders logic while still using asBuildConfigString, asManifestPlaceholder and secretsProperties.requireString.
13-16: Add an explicit existence check forsecrets.propertieswith a clear error.Current load path fails with a generic exception if the file is absent. A pre-check with a targeted message will make local setup and CI failures easier to diagnose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/build.gradle.kts` around lines 13 - 16, The code currently attempts to load secrets.properties without checking existence; update the block around secretsPropertiesFile and secretsProperties to first check secretsPropertiesFile.exists() and throw a clear Gradle error (e.g., throw GradleException or error("secrets.properties not found: ...")) with guidance about creating the file or pointing to CI secrets, before calling secretsPropertiesFile.inputStream().use(::load); keep the Properties().apply pattern but move the existence guard so the load only runs when the file is present.
🤖 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/build.gradle.kts`:
- Around line 68-75: In the signingConfigs create("release") block, replace the
unsafe signingProperties["..."].toString() calls with the requireString() helper
so missing keys fail fast; specifically use
signingProperties.requireString("KEYSTORE_PATH"),
requireString("KEYSTORE_PASSWORD"), requireString("KEY_ALIAS"), and
requireString("KEY_PASSWORD") when assigning
storeFile/storePassword/keyAlias/keyPassword in the create("release") block to
match how secretsProperties is handled.
In `@app/src/main/java/com/cornellappdev/transit/util/TimeUtils.kt`:
- Around line 238-248: The early return when todaySchedule contains "Closed" can
incorrectly treat a venue as closed even if a yesterday overnight interval spans
into the current date; to fix, compute yesterdayOvernightIntervals (using
buildIntervalsForDate with overnightOnly = true and
rotatedOperatingHours.last().hours) and check whether any of those intervals
contain the current date/time before doing the closed-day fallback
(findOpenNextDay); if an overnight interval covers currentDateTime, proceed with
normal open/interval handling instead of returning findOpenNextDay.
In `@gradle.properties`:
- Around line 27-28: Remove the machine-specific org.gradle.java.home entry from
gradle.properties (the hardcoded /Applications/Android Studio... path) so the
repo no longer pins a local macOS JDK; instead delete or comment out the
org.gradle.java.home line and document that contributors should set this in
their user ~/.gradle/gradle.properties or rely on Gradle toolchains / JAVA_HOME.
Ensure no replacement hardcoded path is committed and update any README or CI
config to indicate how to configure JAVA_HOME or toolchain if needed.
---
Nitpick comments:
In `@app/build.gradle.kts`:
- Around line 80-117: The debug/ecosystem/release blocks duplicate wiring of
BACKEND_URL/EATERY_URL/UPLIFT_URL and manifestPlaceholders; extract a helper
function (e.g., configureUrlsForVariant(prefix: String, target:
com.android.build.gradle.internal.dsl.BuildType) or an extension on
DefaultConfig/BuildType) that calls buildConfigField("String", "BACKEND_URL",
asBuildConfigString(secretsProperties.requireString("${prefix}_BACKEND_URL"))),
manifestPlaceholders["BACKEND_URL"] = asManifestPlaceholder(...), and similar
for EATERY_URL and UPLIFT_URL, then call that helper from the debug,
create("ecosystem") and release blocks (keep ECOSYSTEM_FLAG where needed) to
remove the repeated buildConfigField/manifestPlaceholders logic while still
using asBuildConfigString, asManifestPlaceholder and
secretsProperties.requireString.
- Around line 13-16: The code currently attempts to load secrets.properties
without checking existence; update the block around secretsPropertiesFile and
secretsProperties to first check secretsPropertiesFile.exists() and throw a
clear Gradle error (e.g., throw GradleException or error("secrets.properties not
found: ...")) with guidance about creating the file or pointing to CI secrets,
before calling secretsPropertiesFile.inputStream().use(::load); keep the
Properties().apply pattern but move the existence guard so the load only runs
when the file is present.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2c71698-9682-455f-8c8a-ad9d307cdd87
📒 Files selected for processing (7)
.gitignoreapp/build.gradle.ktsapp/release/app-release.aabapp/src/main/AndroidManifest.xmlapp/src/main/java/com/cornellappdev/transit/util/TimeUtils.ktbuild.gradle.ktsgradle.properties
| android.defaults.buildfeatures.buildconfig=true | ||
|
|
||
| # Gradle JDK Configuration | ||
| org.gradle.java.home=/Applications/Android Studio.app/Contents/jbr/Contents/Home |
Member
There was a problem hiding this comment.
+1 for coderabbit message. need to support window devs
| <meta-data | ||
| android:name="BACKEND_URL" | ||
| android:value="${BACKEND_URL}" /> | ||
| <meta-data |
Member
There was a problem hiding this comment.
why are we setting this to false?
| create("ecosystem") { | ||
| initWith(getByName("debug")) | ||
| isDebuggable = true | ||
| buildConfigField( |
Member
There was a problem hiding this comment.
build config field for ecosystem is pretty much the same as debug, so we can consider making a helper
- Add explicit existence check for secrets.properties with clear error message - Replace unsafe signing properties .toString() calls with requireString() helper - Extract helper function to reduce duplicated build-type URL wiring - Fix TimeUtils overnight interval logic to check yesterday's overnight intervals before early return for closed days - Remove machine-specific org.gradle.java.home configuration from gradle.properties
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.
Overview
Changes Made
Test Coverage
Related PRs or Issues
Screenshots
Screenshot taken after 12am
Summary by CodeRabbit
Bug Fixes
Chores
Behavior