Implement price alerts feature and subscription management#28
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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: 3
🧹 Nitpick comments (3)
src/Alerts.js (2)
108-128: 💤 Low valueEmail validation regex could be more robust.
The email validation regex
/^\S+@\S+\.\S+$/(line 110) is very permissive and would accept inputs likea@b.cortest@..test. Consider using a more robust regex or a validation library to catch obviously malformed addresses before submission.Suggested improvement
Use a more comprehensive email regex:
- } else if (!/^\S+@\S+\.\S+$/.test(email.trim())) { + } else if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email.trim())) {Or consider a more thorough pattern that handles edge cases:
} else if (!/^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/.test(email.trim())) {🤖 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 `@src/Alerts.js` around lines 108 - 128, The current email check in Alerts.js uses a too-permissive regex (the /^\S+@\S+\.\S+$/ test against email.trim())—replace it with a stricter pattern or a validation helper: either swap the regex to a more robust one (for example one that enforces valid local and domain characters and a minimum 2-char TLD) or import/use a lightweight validator (e.g., validator.isEmail) where the validation logic around email, email.trim(), and errorText is set; update the condition that sets errorText ("Invalid email address") to use that stricter validation (refer to the email variable and the existing branch that currently runs the /^\S+@\S+\.\S+$/ test).
82-98: 💤 Low valueConsider validating date ranges during URL parameter hydration.
The date hydration from URL params (lines 84-89) only checks if dates are valid but doesn't verify they're within allowed ranges (e.g., start date should be between today and today+30 days). This could prefill dates that immediately show validation errors, creating a confusing UX.
Optional enhancement
Add range validation during hydration:
const s = searchParams.get("start"); const e = searchParams.get("end"); + const today = dayjs.utc().startOf("d"); + const maxStart = today.add(30, "d"); if (s && dayjs(s).isValid()) { - setStartDate(dayjs(s).utc().startOf("d")); + const parsed = dayjs(s).utc().startOf("d"); + if (parsed.isSameOrAfter(today.add(1, "d")) && parsed.isSameOrBefore(maxStart)) { + setStartDate(parsed); + } } if (e && dayjs(e).isValid()) { - setEndDate(dayjs(e).utc().startOf("d")); + const parsed = dayjs(e).utc().startOf("d"); + if (parsed.isSameOrAfter(today.add(1, "d"))) { + setEndDate(parsed); + } }🤖 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 `@src/Alerts.js` around lines 82 - 98, URL param hydration sets start/end dates without enforcing allowed ranges, which can prefill invalid values; update the hydration logic around setStartDate and setEndDate to validate that parsed dates are within the allowed window (e.g., >= today and <= today.plus(30 days)) before calling setStartDate/setEndDate, and if out of range either clamp to the nearest valid date or skip setting them; keep existing validity checks (dayjs(...).isValid()) and apply the same range check for price before calling setPriceThreshold (use parseFloat result) and preserve existing accommodation lookup via ACCOMMODATIONS and setAccommodation.src/DateRangePopover.js (1)
447-451: 💤 Low valueConsider if
!importantis necessary for border style override.The new
sxprop appliesborderStyle: "dashed none !important"to day cells. While!importantis generally discouraged, MUI style overrides sometimes require it to override specificity. However, thesxprop typically has high enough specificity without!important.Try removing
!importantto see if the styles apply correctly:sx={{ ".MuiPickersDay-root": { - borderStyle: "dashed none !important", + borderStyle: "dashed none", }, }}If the styles don't apply without
!important, the current implementation is correct.🤖 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 `@src/DateRangePopover.js` around lines 447 - 451, The sx override in DateRangePopover applies borderStyle: "dashed none !important" to ".MuiPickersDay-root"; remove the "!important" from that borderStyle in the sx prop inside the DateRangePopover component and test the picker to see if the dashed border is applied without it, and only restore "!important" if removing it fails to override MUI styles; target the sx block referencing ".MuiPickersDay-root" when making this change.
🤖 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 `@src/Alerts.js`:
- Around line 60-80: Add an error state for station loading and set it in the
fetch catch so the UI can show feedback: introduce a state like stationsError
with setter setStationsError (alongside existing setStations and
setStationsLoaded), update the catch handler in the useEffect that loads
stations to call setStationsError(true) (and optionally setStations([]) and
setStationsLoaded(true)), and clear setStationsError(false) on a successful
fetch. Then render a user-facing message (e.g., an Alert) in the Alerts
component before the form that checks stationsError and displays "Failed to load
stations. Please refresh the page." so users see the failure instead of empty
Autocomplete dropdowns.
In `@src/Fares.js`:
- Around line 869-882: The URL passed to the Button's Link prop (the "to" prop
in the Button component usage) currently contains literal newlines and unencoded
query values; change the construction so the template does not span lines (no
embedded newlines) and URL-encode each query value (origin.code,
destination.code, fareClass) and the formatted dates
(dateRangeStart.format(...), dateRangeEnd.format(...)) — e.g., build the query
string with encodeURIComponent for each value or use URLSearchParams to generate
the query portion, then concatenate with `/alerts?` and pass that single-line
encoded string to the to prop.
In `@src/Navbar.js`:
- Around line 26-30: The mobile label for the Alerts link is rendering a leading
space because the ternary uses " " as the mobile prefix; update the rendering
logic in the Navbar component (the Link that uses useLocation().pathname and the
ternary that produces `{!mobile ? "Fare " : " "}`) to use an empty string "" for
the mobile case (or compute the label once based on useLocation() and mobile and
return either "Fare Alerts", "Alerts", or "Home" accordingly) so mobile displays
"Alerts" without a leading space.
---
Nitpick comments:
In `@src/Alerts.js`:
- Around line 108-128: The current email check in Alerts.js uses a
too-permissive regex (the /^\S+@\S+\.\S+$/ test against email.trim())—replace it
with a stricter pattern or a validation helper: either swap the regex to a more
robust one (for example one that enforces valid local and domain characters and
a minimum 2-char TLD) or import/use a lightweight validator (e.g.,
validator.isEmail) where the validation logic around email, email.trim(), and
errorText is set; update the condition that sets errorText ("Invalid email
address") to use that stricter validation (refer to the email variable and the
existing branch that currently runs the /^\S+@\S+\.\S+$/ test).
- Around line 82-98: URL param hydration sets start/end dates without enforcing
allowed ranges, which can prefill invalid values; update the hydration logic
around setStartDate and setEndDate to validate that parsed dates are within the
allowed window (e.g., >= today and <= today.plus(30 days)) before calling
setStartDate/setEndDate, and if out of range either clamp to the nearest valid
date or skip setting them; keep existing validity checks (dayjs(...).isValid())
and apply the same range check for price before calling setPriceThreshold (use
parseFloat result) and preserve existing accommodation lookup via ACCOMMODATIONS
and setAccommodation.
In `@src/DateRangePopover.js`:
- Around line 447-451: The sx override in DateRangePopover applies borderStyle:
"dashed none !important" to ".MuiPickersDay-root"; remove the "!important" from
that borderStyle in the sx prop inside the DateRangePopover component and test
the picker to see if the dashed border is applied without it, and only restore
"!important" if removing it fails to override MUI styles; target the sx block
referencing ".MuiPickersDay-root" when making this change.
🪄 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 Plus
Run ID: bb7bd747-bf68-428c-af84-595b20d30f75
📒 Files selected for processing (11)
src/Alerts.csssrc/Alerts.jssrc/AppRouter.jssrc/DateRangePopover.csssrc/DateRangePopover.jssrc/Fares.jssrc/Form.jssrc/Navbar.csssrc/Navbar.jssrc/Subscribed.jssrc/Unsubscribed.js
💤 Files with no reviewable changes (1)
- src/DateRangePopover.css
Comment on lines
+60
to
+80
| useEffect(() => { | ||
| fetch(`${process.env.REACT_APP_API_DOMAIN}/stations`) | ||
| .then((res) => res.json()) | ||
| .then((data) => { | ||
| const sorted = data | ||
| .sort((a, b) => a.stateLong.localeCompare(b.stateLong)) | ||
| .map((station) => ({ ...station, group: station.stateLong })); | ||
| setStations(sorted); | ||
| const o = searchParams.get("origin"); | ||
| const d = searchParams.get("destination"); | ||
| if (o) { | ||
| const match = sorted.find((s) => s.code === o); | ||
| if (match) setOrigin(match); | ||
| } | ||
| if (d) { | ||
| const match = sorted.find((s) => s.code === d); | ||
| if (match) setDestination(match); | ||
| } | ||
| setStationsLoaded(true); | ||
| }) | ||
| .catch(() => setStationsLoaded(true)); |
Contributor
There was a problem hiding this comment.
Add error feedback when station loading fails.
If the stations API call fails (line 80), stationsLoaded is set to true but the stations array remains empty. This enables the Autocomplete dropdowns with no options, leaving users unable to select stations without any indication of what went wrong.
💡 Suggested improvement
Add an error state and display a message to the user:
const [stations, setStations] = useState([]);
const [stationsLoaded, setStationsLoaded] = useState(false);
+ const [stationsError, setStationsError] = useState(false);
useEffect(() => {
fetch(`${process.env.REACT_APP_API_DOMAIN}/stations`)
.then((res) => res.json())
.then((data) => {
const sorted = data
.sort((a, b) => a.stateLong.localeCompare(b.stateLong))
.map((station) => ({ ...station, group: station.stateLong }));
setStations(sorted);
// ... rest of hydration logic
setStationsLoaded(true);
})
- .catch(() => setStationsLoaded(true));
+ .catch(() => {
+ setStationsLoaded(true);
+ setStationsError(true);
+ });Then display the error in the UI before the form, e.g.:
{stationsError && (
<Alert severity="error">
Failed to load stations. Please refresh the page.
</Alert>
)}🤖 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 `@src/Alerts.js` around lines 60 - 80, Add an error state for station loading
and set it in the fetch catch so the UI can show feedback: introduce a state
like stationsError with setter setStationsError (alongside existing setStations
and setStationsLoaded), update the catch handler in the useEffect that loads
stations to call setStationsError(true) (and optionally setStations([]) and
setStationsLoaded(true)), and clear setStationsError(false) on a successful
fetch. Then render a user-facing message (e.g., an Alert) in the Alerts
component before the form that checks stationsError and displays "Failed to load
stations. Please refresh the page." so users see the failure instead of empty
Autocomplete dropdowns.
Comment on lines
+869
to
+882
| <Button | ||
| component={Link} | ||
| data-rybbit-event="alert" | ||
| endIcon={<NotificationsActiveIcon />} | ||
| to={`/alerts?origin=${origin.code}&destination=${ | ||
| destination.code | ||
| }&start=${dateRangeStart.format( | ||
| "YYYY-MM-DD" | ||
| )}&end=${dateRangeEnd.format("YYYY-MM-DD")} | ||
| &accommodation=${fareClass}`} | ||
| variant="outlined" | ||
| > | ||
| Alert me | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Fix URL construction: remove literal newlines and add URL encoding.
The template literal spans multiple lines (873-878), which embeds literal newline characters into the URL string. This will result in a malformed URL or newlines encoded as %0A. Additionally, query parameter values (origin.code, destination.code, fareClass) are not URL-encoded, which will break if they contain special characters like spaces, &, or =.
🔗 Proposed fix
<Button
component={Link}
data-rybbit-event="alert"
endIcon={<NotificationsActiveIcon />}
- to={`/alerts?origin=${origin.code}&destination=${
- destination.code
- }&start=${dateRangeStart.format(
- "YYYY-MM-DD"
- )}&end=${dateRangeEnd.format("YYYY-MM-DD")}
- &accommodation=${fareClass}`}
+ to={`/alerts?origin=${encodeURIComponent(origin.code)}&destination=${encodeURIComponent(destination.code)}&start=${dateRangeStart.format("YYYY-MM-DD")}&end=${dateRangeEnd.format("YYYY-MM-DD")}&accommodation=${encodeURIComponent(fareClass)}`}
variant="outlined"
>
Alert me🤖 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 `@src/Fares.js` around lines 869 - 882, The URL passed to the Button's Link
prop (the "to" prop in the Button component usage) currently contains literal
newlines and unencoded query values; change the construction so the template
does not span lines (no embedded newlines) and URL-encode each query value
(origin.code, destination.code, fareClass) and the formatted dates
(dateRangeStart.format(...), dateRangeEnd.format(...)) — e.g., build the query
string with encodeURIComponent for each value or use URLSearchParams to generate
the query portion, then concatenate with `/alerts?` and pass that single-line
encoded string to the to prop.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
This pull request introduces the core UI and routing for a new "Price Alerts" feature, allowing users to set fare alerts and manage their subscription status. Key changes include new alert-related pages, navigation updates, a call-to-action button in the fares view, and a new CSS file for consistent alert styling.
Feature implementation and routing:
Alerts,Subscribed, andUnsubscribed, and registered their routes inAppRouter.jsto handle alert creation and subscription status feedback. [1] [2] [3] [4]User interface enhancements:
Fares.js, enabling users to quickly set a price alert for their selected trip and dates. [1] [2] [3]Navbar.js) to include a "Price Alerts" link, improving discoverability of the new feature.Styling:
Alerts.cssfile to style all alert-related components, ensuring a consistent and responsive design across alert pages.Configuration:
Form.jsto use an environment variable, improving security and configurability.Summary by CodeRabbit
New Features
Bug Fixes