Add initial UI for hotspot implementation [Post-Ecosystem]#142
Open
RyanCheung555 wants to merge 5 commits intomainfrom
Open
Add initial UI for hotspot implementation [Post-Ecosystem]#142RyanCheung555 wants to merge 5 commits intomainfrom
RyanCheung555 wants to merge 5 commits intomainfrom
Conversation
Merge branch 'main' into ryan/hotspot-page
📝 WalkthroughWalkthroughA new "Hotspots" feature is introduced with a request sheet for user submissions, new map markers, and a reusable Changes
Sequence DiagramsequenceDiagram
actor User
participant HomeScreen
participant ViewModel
participant RequestHotspotSheet
participant System as System/Toast
User->>HomeScreen: Tap "Request Hotspot" in ecosystem sheet
HomeScreen->>ViewModel: Call onRequestHotspotClick()
ViewModel->>HomeScreen: Set showRequestHotspotSheet = true
HomeScreen->>RequestHotspotSheet: Show ModalBottomSheet
User->>RequestHotspotSheet: Fill form (name, netId, type, desc, location)
User->>RequestHotspotSheet: Tap Submit button
RequestHotspotSheet->>HomeScreen: Invoke onSubmit callback
HomeScreen->>ViewModel: Call toggleRequestHotspotSheet(false)
ViewModel->>HomeScreen: Update showRequestHotspotSheet state
HomeScreen->>RequestHotspotSheet: Hide sheet
HomeScreen->>System: Display "Hotspot request submitted" toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/src/main/java/com/cornellappdev/transit/ui/components/home/PillButton.kt (1)
27-68: Reusable API considerations forPillButton.A few small concerns about the reusable surface:
modifier.fillMaxWidth().height(40.dp)applies size modifiers after the caller'smodifier, so any size set by callers is silently overridden. Consider letting callers control sizing, or at minimum apply caller's modifier last.- Defaulting
iconResIdtoR.drawable.ic_additionbakes a feature-specific asset into a generic component. Since this is now used for both "Add Favorites" and "Request Hotspot" (which presumably wants a different icon), consider making it required (no default) or defaulting tonull.contentDescriptiondefaults totext, which causes TalkBack to announce the icon label and then the text label — effectively a duplicated announcement. For decorative icons next to a label,contentDescription = nullis typically preferred.♻️ Proposed refactor
fun PillButton( onClick: () -> Unit, text: String, modifier: Modifier = Modifier, - `@DrawableRes` iconResId: Int? = R.drawable.ic_addition, - contentDescription: String = text, + `@DrawableRes` iconResId: Int? = null, + contentDescription: String? = null, colors: ButtonColors = ButtonDefaults.buttonColors( containerColor = Color.White, contentColor = Color.Black ), ... ) { Button( onClick = onClick, colors = colors, - modifier = modifier - .fillMaxWidth() - .height(40.dp), + modifier = Modifier + .fillMaxWidth() + .height(40.dp) + .then(modifier), ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/transit/ui/components/home/PillButton.kt` around lines 27 - 68, PillButton currently forces sizing and a non-null default icon and duplicates accessibility text: move the size constraints off the caller's Modifier so the passed-in modifier is applied last (ensure modifier.then(Modifier.fillMaxWidth().height(40.dp)) or better, remove forced sizing and document sizing behavior) so callers can override size; change iconResId default from R.drawable.ic_addition to null (or make iconResId required) to avoid baking a feature-specific asset into the reusable component; and set the icon's contentDescription default to null (or use a separate iconContentDescription param) so decorative icons don't duplicate the Text announcement; update the Icon usage in PillButton to only supply contentDescription when non-null.app/src/main/java/com/cornellappdev/transit/ui/components/home/HomeScreenMarkers.kt (1)
29-32: Move placeholder list into theHOTSPOTSbranch (or a top-levelval).
placeholderHotspotMarkersis allocated on every recomposition ofHomeScreenMarkers, even when the currentfilterStateis notHOTSPOTS. Since its contents are constant, hoist it to a top-levelprivate valor at least scope it inside theFilterState.HOTSPOTSbranch to avoid unnecessary allocation.Also — per the PR's "Next steps", remember to replace these hardcoded coordinates with real data before shipping. Adding a
TODOcomment here would make that easier to track.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/transit/ui/components/home/HomeScreenMarkers.kt` around lines 29 - 32, placeholderHotspotMarkers is being reallocated on every recomposition of HomeScreenMarkers; move its constant list into the FilterState.HOTSPOTS branch or hoist it as a top-level private val (e.g., private val PLACEHOLDER_HOTSPOT_MARKERS) so it’s not recreated each recomposition, and add a TODO noting these hardcoded coordinates must be replaced with real data before shipping; update references in HomeScreenMarkers to use the hoisted/branch-scoped symbol instead of the local variable.
🤖 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/cornellappdev/transit/ui/components/home/RequestHotspotSheet.kt`:
- Around line 55-64: The sheet currently captures form fields (name, netId,
eventType, description, location) but calls onSubmit with no data; update the
RequestHotspotSheet API to accept a request payload (e.g., a HotspotRequest or
similar) and change the onSubmit parameter to onSubmit: (HotspotRequest) ->
Unit; before invoking onSubmit, validate required fields (at minimum name and
netId and eventType) and only call onSubmit with a constructed HotspotRequest
when validation passes (otherwise show an error UI or disable the submit
action); apply the same change where the sheet is reused (the other
RequestHotspotSheet usage around the block referenced 222-231) so callers
receive the filled model.
- Around line 210-219: The PillButton for "Add Photos" is a tappable no-op;
change it so it is not interactive until the picker is implemented by either
removing the active onClick handler or disabling/hiding the control: update the
PillButton usage (the PillButton call with text "Add Photos" and current onClick
= {}) to either (a) set the button non-interactive (use the PillButton's
enabled/disabled API or equivalent) and optionally change the label to "Add
Photos (coming soon)", or (b) conditionally render nothing until the photo
picker is wired; ensure you only modify the PillButton invocation (the onClick
parameter and/or enabled/visibility) so no empty callback remains.
In `@app/src/main/java/com/cornellappdev/transit/ui/screens/HomeScreen.kt`:
- Around line 514-521: The onSubmit handler currently closes the sheet
(homeViewModel.toggleRequestHotspotSheet(false)) and shows a success Toast even
though backend submission is TODO; stop asserting success until submission is
implemented by removing or changing those actions: do not call
homeViewModel.toggleRequestHotspotSheet(false) nor show the "Hotspot request
submitted" Toast inside the onSubmit lambda; instead keep the sheet open (or
show a neutral message like "Submission not implemented" via Toast/Snackbar) and
only call toggleRequestHotspotSheet(false) and show a success Toast after the
actual network request completes successfully in the future submission flow.
---
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/transit/ui/components/home/HomeScreenMarkers.kt`:
- Around line 29-32: placeholderHotspotMarkers is being reallocated on every
recomposition of HomeScreenMarkers; move its constant list into the
FilterState.HOTSPOTS branch or hoist it as a top-level private val (e.g.,
private val PLACEHOLDER_HOTSPOT_MARKERS) so it’s not recreated each
recomposition, and add a TODO noting these hardcoded coordinates must be
replaced with real data before shipping; update references in HomeScreenMarkers
to use the hoisted/branch-scoped symbol instead of the local variable.
In
`@app/src/main/java/com/cornellappdev/transit/ui/components/home/PillButton.kt`:
- Around line 27-68: PillButton currently forces sizing and a non-null default
icon and duplicates accessibility text: move the size constraints off the
caller's Modifier so the passed-in modifier is applied last (ensure
modifier.then(Modifier.fillMaxWidth().height(40.dp)) or better, remove forced
sizing and document sizing behavior) so callers can override size; change
iconResId default from R.drawable.ic_addition to null (or make iconResId
required) to avoid baking a feature-specific asset into the reusable component;
and set the icon's contentDescription default to null (or use a separate
iconContentDescription param) so decorative icons don't duplicate the Text
announcement; update the Icon usage in PillButton to only supply
contentDescription when non-null.
🪄 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: b9a6b9e6-a65c-4b6b-8eca-71c7853e66d1
📒 Files selected for processing (12)
app/src/main/java/com/cornellappdev/transit/ui/components/home/AddFavoritesButton.ktapp/src/main/java/com/cornellappdev/transit/ui/components/home/EcosystemBottomSheetContent.ktapp/src/main/java/com/cornellappdev/transit/ui/components/home/HomeScreenMarkers.ktapp/src/main/java/com/cornellappdev/transit/ui/components/home/PillButton.ktapp/src/main/java/com/cornellappdev/transit/ui/components/home/RequestHotspotSheet.ktapp/src/main/java/com/cornellappdev/transit/ui/screens/HomeScreen.ktapp/src/main/java/com/cornellappdev/transit/ui/theme/Color.ktapp/src/main/java/com/cornellappdev/transit/ui/viewmodels/FilterState.ktapp/src/main/java/com/cornellappdev/transit/ui/viewmodels/HomeViewModel.ktapp/src/main/res/drawable/hotspot_event_pin.xmlapp/src/main/res/drawable/hotspot_fun_spot_pin.xmlapp/src/main/res/drawable/hotspot_icon.xml
💤 Files with no reviewable changes (1)
- app/src/main/java/com/cornellappdev/transit/ui/components/home/AddFavoritesButton.kt
Comment on lines
+55
to
+64
| fun RequestHotspotSheet( | ||
| onDismiss: () -> Unit, | ||
| onSubmit: () -> Unit, | ||
| modifier: Modifier = Modifier, | ||
| ) { | ||
| var name by remember { mutableStateOf("") } | ||
| var netId by remember { mutableStateOf("") } | ||
| var eventType by remember { mutableStateOf("") } | ||
| var description by remember { mutableStateOf("") } | ||
| var location by remember { mutableStateOf("") } |
There was a problem hiding this comment.
Submit should pass the entered request data.
The sheet owns all form fields, but onSubmit has no payload, so callers cannot persist what the user entered. Pass a request model and gate obvious empty required fields before invoking submit.
🐛 Proposed submit API shape
+data class HotspotRequestInput(
+ val name: String,
+ val netId: String,
+ val eventType: String,
+ val description: String,
+ val location: String,
+)
+
`@OptIn`(ExperimentalMaterial3Api::class)
`@Composable`
fun RequestHotspotSheet(
onDismiss: () -> Unit,
- onSubmit: () -> Unit,
+ onSubmit: (HotspotRequestInput) -> Unit,
modifier: Modifier = Modifier,
) {
var name by remember { mutableStateOf("") }
var netId by remember { mutableStateOf("") }
var eventType by remember { mutableStateOf("") }
var description by remember { mutableStateOf("") }
var location by remember { mutableStateOf("") }
+ val canSubmit = name.isNotBlank() &&
+ netId.isNotBlank() &&
+ eventType.isNotBlank() &&
+ location.isNotBlank() PillButton(
- onClick = onSubmit,
+ onClick = {
+ if (!canSubmit) return@PillButton
+
+ onSubmit(
+ HotspotRequestInput(
+ name = name.trim(),
+ netId = netId.trim(),
+ eventType = eventType,
+ description = description.trim(),
+ location = location.trim(),
+ )
+ )
+ },
text = "Submit",
iconResId = null,Also applies to: 222-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/transit/ui/components/home/RequestHotspotSheet.kt`
around lines 55 - 64, The sheet currently captures form fields (name, netId,
eventType, description, location) but calls onSubmit with no data; update the
RequestHotspotSheet API to accept a request payload (e.g., a HotspotRequest or
similar) and change the onSubmit parameter to onSubmit: (HotspotRequest) ->
Unit; before invoking onSubmit, validate required fields (at minimum name and
netId and eventType) and only call onSubmit with a constructed HotspotRequest
when validation passes (otherwise show an error UI or disable the submit
action); apply the same change where the sheet is reused (the other
RequestHotspotSheet usage around the block referenced 222-231) so callers
receive the filled model.
Comment on lines
+210
to
+219
| PillButton( | ||
| onClick = {}, | ||
| text = "Add Photos", | ||
| colors = ButtonDefaults.buttonColors( | ||
| containerColor = DividerGray, | ||
| contentColor = SecondaryText | ||
| ), | ||
| textColor = SecondaryText, | ||
| iconTint = SecondaryText | ||
| ) |
There was a problem hiding this comment.
Avoid shipping a tappable no-op.
The “Add Photos” button currently accepts taps but does nothing. Hide it until the picker is wired, or add a real callback.
🧹 Proposed minimal fix until photo upload is implemented
- PillButton(
- onClick = {},
- text = "Add Photos",
- colors = ButtonDefaults.buttonColors(
- containerColor = DividerGray,
- contentColor = SecondaryText
- ),
- textColor = SecondaryText,
- iconTint = SecondaryText
- )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| PillButton( | |
| onClick = {}, | |
| text = "Add Photos", | |
| colors = ButtonDefaults.buttonColors( | |
| containerColor = DividerGray, | |
| contentColor = SecondaryText | |
| ), | |
| textColor = SecondaryText, | |
| iconTint = SecondaryText | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/transit/ui/components/home/RequestHotspotSheet.kt`
around lines 210 - 219, The PillButton for "Add Photos" is a tappable no-op;
change it so it is not interactive until the picker is implemented by either
removing the active onClick handler or disabling/hiding the control: update the
PillButton usage (the PillButton call with text "Add Photos" and current onClick
= {}) to either (a) set the button non-interactive (use the PillButton's
enabled/disabled API or equivalent) and optionally change the label to "Add
Photos (coming soon)", or (b) conditionally render nothing until the photo
picker is wired; ensure you only modify the PillButton invocation (the onClick
parameter and/or enabled/visibility) so no empty callback remains.
Comment on lines
+514
to
+521
| onSubmit = { | ||
| homeViewModel.toggleRequestHotspotSheet(false) | ||
| // TODO: Connect to backend | ||
| Toast.makeText( | ||
| context, | ||
| "Hotspot request submitted", | ||
| Toast.LENGTH_SHORT | ||
| ).show() |
There was a problem hiding this comment.
Don’t show success for a request that is not submitted.
Line 516 still has the backend TODO, but the UI closes the sheet and tells users “Hotspot request submitted.” That drops the request while confirming success.
🐛 Proposed minimal guard until submission is implemented
onSubmit = {
- homeViewModel.toggleRequestHotspotSheet(false)
- // TODO: Connect to backend
+ // TODO: Connect to backend and only show success after persistence succeeds.
Toast.makeText(
context,
- "Hotspot request submitted",
+ "Hotspot request submission is not available yet",
Toast.LENGTH_SHORT
).show()
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onSubmit = { | |
| homeViewModel.toggleRequestHotspotSheet(false) | |
| // TODO: Connect to backend | |
| Toast.makeText( | |
| context, | |
| "Hotspot request submitted", | |
| Toast.LENGTH_SHORT | |
| ).show() | |
| onSubmit = { | |
| // TODO: Connect to backend and only show success after persistence succeeds. | |
| Toast.makeText( | |
| context, | |
| "Hotspot request submission is not available yet", | |
| Toast.LENGTH_SHORT | |
| ).show() | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/cornellappdev/transit/ui/screens/HomeScreen.kt` around
lines 514 - 521, The onSubmit handler currently closes the sheet
(homeViewModel.toggleRequestHotspotSheet(false)) and shows a success Toast even
though backend submission is TODO; stop asserting success until submission is
implemented by removing or changing those actions: do not call
homeViewModel.toggleRequestHotspotSheet(false) nor show the "Hotspot request
submitted" Toast inside the onSubmit lambda; instead keep the sheet open (or
show a neutral message like "Submission not implemented" via Toast/Snackbar) and
only call toggleRequestHotspotSheet(false) and show a success Toast after the
actual network request completes successfully in the future submission flow.
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.
Overview
Changes Made
Test Coverage
Next Steps (delete if not applicable)
Screenshots (delete if not applicable)
Hotspot UI Demo
HotspotUIDemo.webm
Summary by CodeRabbit