Fix/arrival-and-departure-for-stop endpoint#962
Open
3rabiii wants to merge 23 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors arrival/departure handlers for ordinal-based stop selection, distance-based stop finding, SQL to return stop ordinals, request parsing/time localization, prediction/vehicle enrichment, response assembly (0-based ordinals and cancellation handling), model/test fixes, and minor DB row-close cleanup. ChangesArrival & Stop-selection Refactor
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 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: 1
🤖 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 `@internal/restapi/arrival_and_departure_for_stop_handler.go`:
- Around line 373-375: The code is deriving a zero-based stop ordinal from raw
GTFS stop_times.stop_sequence (stopSequenceZeroBased) which can be sparse and
causes mismatches with the lookup that uses params.StopSequence; instead compute
a trip-relative ordinal in the DB/query layer and return that same ordinal with
targetStopTime and totalStopsInTrip so request matching and response use the
same value. Update the call site that uses
GetTargetStopTimeWithTotalStopsBySequence(...) to request and receive a
trip-relative ordinal (not raw stop_sequence), stop using params.StopSequence
directly for lookup, and replace stopSequenceZeroBased, arrivalEnabled and
departureEnabled calculations to use the returned trip-relative ordinal and
totalStopsInTrip (reference symbols: GetTargetStopTimeWithTotalStopsBySequence,
params.StopSequence, targetStopTime, totalStopsInTrip, stopSequenceZeroBased,
arrivalEnabled, departureEnabled).
🪄 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 Plus
Run ID: 5e6ad429-2af9-4ee3-badc-d1d38685fff4
📒 Files selected for processing (6)
gtfsdb/block_layover_test.gogtfsdb/stops_rtree.gointernal/models/arrival_and_departure.gointernal/models/current_time_test.gointernal/models/trip_details_test.gointernal/restapi/arrival_and_departure_for_stop_handler.go
3rabiii
force-pushed
the
fix/arrival-departure-endpoint
branch
from
64f5c18 to
b9228ce
Compare
3rabiii
force-pushed
the
fix/arrival-departure-endpoint
branch
from
1a091ea to
446d15b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
gtfsdb/query.sql (1)
1248-1249: ⚡ Quick winInconsistent parameter style: positional
?3mixed with named@current_time.Line 1248 uses
@current_timebut line 1249 uses the positional?3. This happens to work because sqlc assigns@current_timeto position 3, but it's fragile—if someone reorders the named parameters in the WHERE clause,?3could silently refer to the wrong parameter.♻️ Proposed fix: use consistent named parameter
WHERE st.trip_id = `@trip_id` AND st.stop_id = `@stop_id` AND CAST(`@current_time` AS INTEGER) IS NOT NULL -ORDER BY ABS(st.arrival_time - CAST(?3 AS INTEGER)), st.stop_sequence +ORDER BY ABS(st.arrival_time - CAST(`@current_time` AS INTEGER)), st.stop_sequence🤖 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 `@gtfsdb/query.sql` around lines 1248 - 1249, The ORDER BY mixes positional parameter ?3 with the named parameter `@current_time` (used in the WHERE), which is fragile; change the ORDER BY to use the same named parameter (`@current_time`) instead of ?3 so the query consistently references CAST(`@current_time` AS INTEGER) in the expression ABS(st.arrival_time - CAST(`@current_time` AS INTEGER)), leaving the WHERE clause (st.trip_id = `@trip_id` AND st.stop_id = `@stop_id` AND CAST(`@current_time` AS INTEGER) IS NOT NULL), and preserving ORDER BY st.stop_sequence.internal/restapi/block_sequence_helper.go (1)
35-38: 💤 Low valueConsider caching stop times to avoid redundant DB query.
The first loop (lines 35-38) fetches stop times for each block trip to compute
startTime, and the second loop (lines 61-64) fetches them again for the matching trip. SinceTripWithDetailsonly storesTripIDandStartTime, this results in a redundant DB call for the matched trip.Consider storing
stopTimesinTripWithDetailsto reuse them in the second loop.♻️ Suggested refactor
type TripWithDetails struct { TripID string StartTime int + StopTimes []gtfsdb.StopTime } // ... in first loop ... if startTime != math.MaxInt64 { activeTrips = append(activeTrips, TripWithDetails{ TripID: blockTrip.ID, StartTime: int(startTime), + StopTimes: stopTimes, }) } // ... in second loop, use trip.StopTimes instead of fetching again ... if trip.TripID == tripID { - stopTimes, err := api.GtfsManager.GtfsDB.Queries.GetStopTimesForTrip(ctx, trip.TripID) - if err != nil { - continue - } - for i, st := range stopTimes { + for i, st := range trip.StopTimes { if int(st.StopSequence) == stopSequence { return blockSequence + i } } return blockSequence } -blockSequence += len(stopTimes) +blockSequence += len(trip.StopTimes)Also applies to: 61-64
🤖 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 `@internal/restapi/block_sequence_helper.go` around lines 35 - 38, The code repeatedly calls GetStopTimesForTrip for the same trip; modify TripWithDetails to include a stopTimes field (or pointer) and when you fetch stopTimes the first time (inside the loop using blockTrip.ID) store them on the TripWithDetails instance; then in the later loop that currently re-fetches stop times (the second loop around the matching trip logic) reuse TripWithDetails.stopTimes instead of calling GetStopTimesForTrip again, and guard against nil/empty before computing StartTime/other derived values.internal/restapi/arrival_and_departure_for_stop_handler.go (1)
436-440: 💤 Low valueInconsistent
numberOfStopsAwaydefault between handlers.The singular handler defaults
numberOfStopsAwayto0when the lookup returns nil (line 439), but the plural handler (arrivals_and_departures_for_stop_handler.go, line 371) defaults to-1. This may cause inconsistent API responses depending on which endpoint is called.If this is intentional per the API spec, please add a comment explaining the rationale. Otherwise, consider aligning the default value.
🤖 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 `@internal/restapi/arrival_and_departure_for_stop_handler.go` around lines 436 - 440, The singular handler in internal/restapi/arrival_and_departure_for_stop_handler.go sets numberOfStopsAway to 0 when numberOfStopsAwayPtr is nil while the plural handler in arrivals_and_departures_for_stop_handler.go uses -1, causing inconsistent API responses; either (A) make the singular handler use the same default (-1) as the plural handler by changing the nil branch for numberOfStopsAway to -1 so both handlers are aligned, or (B) if the difference is intentional per the API spec, add a concise inline comment next to the nil-handling branch in arrival_and_departure_for_stop_handler.go referencing the spec and explaining why 0 differs from the plural handler (include the same comment in arrivals_and_departures_for_stop_handler.go if needed for clarity).
🤖 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 `@internal/restapi/arrival_and_departure_for_stop_handler.go`:
- Around line 823-850: The getNumberOfStopsAway logic must treat a missing block
sequence explicitly because getBlockSequenceForStopSequence can return 0 for
“not found”; update getNumberOfStopsAway to detect a missing targetGlobalSeq
(e.g., if targetGlobalSeq == 0) and return nil immediately instead of computing
deltas that can be misleading, and adjust downstream checks similarly when
calling getBlockSequenceForStopID (or alter getBlockSequenceForStopSequence to
return a clear sentinel like -1 on not-found and handle that sentinel here);
modify the code paths in getNumberOfStopsAway (references: getNumberOfStopsAway,
getBlockSequenceForStopSequence, getBlockSequenceForStopID,
rawSequenceToOrdinal) so missing sequences lead to nil rather than producing
negative/incorrect numberOfStopsAway.
---
Nitpick comments:
In `@gtfsdb/query.sql`:
- Around line 1248-1249: The ORDER BY mixes positional parameter ?3 with the
named parameter `@current_time` (used in the WHERE), which is fragile; change the
ORDER BY to use the same named parameter (`@current_time`) instead of ?3 so the
query consistently references CAST(`@current_time` AS INTEGER) in the expression
ABS(st.arrival_time - CAST(`@current_time` AS INTEGER)), leaving the WHERE clause
(st.trip_id = `@trip_id` AND st.stop_id = `@stop_id` AND CAST(`@current_time` AS
INTEGER) IS NOT NULL), and preserving ORDER BY st.stop_sequence.
In `@internal/restapi/arrival_and_departure_for_stop_handler.go`:
- Around line 436-440: The singular handler in
internal/restapi/arrival_and_departure_for_stop_handler.go sets
numberOfStopsAway to 0 when numberOfStopsAwayPtr is nil while the plural handler
in arrivals_and_departures_for_stop_handler.go uses -1, causing inconsistent API
responses; either (A) make the singular handler use the same default (-1) as the
plural handler by changing the nil branch for numberOfStopsAway to -1 so both
handlers are aligned, or (B) if the difference is intentional per the API spec,
add a concise inline comment next to the nil-handling branch in
arrival_and_departure_for_stop_handler.go referencing the spec and explaining
why 0 differs from the plural handler (include the same comment in
arrivals_and_departures_for_stop_handler.go if needed for clarity).
In `@internal/restapi/block_sequence_helper.go`:
- Around line 35-38: The code repeatedly calls GetStopTimesForTrip for the same
trip; modify TripWithDetails to include a stopTimes field (or pointer) and when
you fetch stopTimes the first time (inside the loop using blockTrip.ID) store
them on the TripWithDetails instance; then in the later loop that currently
re-fetches stop times (the second loop around the matching trip logic) reuse
TripWithDetails.stopTimes instead of calling GetStopTimesForTrip again, and
guard against nil/empty before computing StartTime/other derived values.
🪄 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 Plus
Run ID: a474d4ca-6b2e-40eb-bb98-9ce086a68f64
📒 Files selected for processing (15)
gtfsdb/block_layover_test.gogtfsdb/query.sqlgtfsdb/query.sql.gogtfsdb/stops_rtree.gointernal/models/arrival_and_departure.gointernal/models/current_time_test.gointernal/models/trip_details.gointernal/models/trip_details_test.gointernal/restapi/arrival_and_departure_for_stop_handler.gointernal/restapi/arrival_and_departure_for_stop_handler_test.gointernal/restapi/arrivals_and_departures_for_stop_handler.gointernal/restapi/block_sequence_helper.gointernal/restapi/calculate_block_trip_sequence_test.gointernal/restapi/trips_helper.gointernal/restapi/trips_helper_test.go
✅ Files skipped from review due to trivial changes (1)
- gtfsdb/stops_rtree.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/restapi/arrival_and_departure_for_stop_handler.go (1)
842-847:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback path missing sentinel checks for
-1return values.The change at line 835 correctly guards against
-1sentinels fromgetBlockSequenceForStopID, but the fallback path at lines 844-846 doesn't apply the same guard. IftargetGlobalSeq == -1(which we know is possible since line 835 checks for it) orvehicleGlobalSeq == -1, the computednumberOfStopsAwaywill be incorrect (e.g.,-1 - 5 = -6or10 - (-1) = 11).🐛 Proposed fix to add sentinel checks in fallback path
currentVehicleStopSequence := getCurrentVehicleStopSequence(vehicle) if currentVehicleStopSequence != nil { vehicleGlobalSeq := api.getBlockSequenceForStopSequence(ctx, activeTripID, *currentVehicleStopSequence, serviceDate) + if vehicleGlobalSeq == -1 || targetGlobalSeq == -1 { + return nil + } numberOfStopsAway := targetGlobalSeq - vehicleGlobalSeq return &numberOfStopsAway }🤖 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 `@internal/restapi/arrival_and_departure_for_stop_handler.go` around lines 842 - 847, The fallback path computes numberOfStopsAway using api.getBlockSequenceForStopSequence and getCurrentVehicleStopSequence but omits sentinel checks; update the block where currentVehicleStopSequence is used (calls to getCurrentVehicleStopSequence and api.getBlockSequenceForStopSequence) to verify that both targetGlobalSeq and vehicleGlobalSeq are not -1 before computing numberOfStopsAway and returning a pointer; if either is -1, return nil (same behavior as the guarded path), ensuring you reference targetGlobalSeq, vehicleGlobalSeq, getCurrentVehicleStopSequence, api.getBlockSequenceForStopSequence, and numberOfStopsAway when making the checks.
🤖 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 `@internal/restapi/block_sequence_helper.go`:
- Line 19: Don't return -1 from the error branch of GetTripsByBlockID; instead
preserve the recoverable fallback path by handling the error (log or wrap it)
and proceeding to the existing raw trip-level fallback function (the code that
handles trip-level lookup already present in this file). Replace the early
return in the GetTripsByBlockID error case with a call into the raw trip-level
fallback logic (or a call to the trip-level lookup helper used elsewhere in
block_sequence_helper.go) so transient block lookup failures fall back to
trip-level resolution rather than terminating with -1.
---
Outside diff comments:
In `@internal/restapi/arrival_and_departure_for_stop_handler.go`:
- Around line 842-847: The fallback path computes numberOfStopsAway using
api.getBlockSequenceForStopSequence and getCurrentVehicleStopSequence but omits
sentinel checks; update the block where currentVehicleStopSequence is used
(calls to getCurrentVehicleStopSequence and api.getBlockSequenceForStopSequence)
to verify that both targetGlobalSeq and vehicleGlobalSeq are not -1 before
computing numberOfStopsAway and returning a pointer; if either is -1, return nil
(same behavior as the guarded path), ensuring you reference targetGlobalSeq,
vehicleGlobalSeq, getCurrentVehicleStopSequence,
api.getBlockSequenceForStopSequence, and numberOfStopsAway when making the
checks.
🪄 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 Plus
Run ID: 80118235-04c3-4639-a5d9-35d38799e23d
📒 Files selected for processing (2)
internal/restapi/arrival_and_departure_for_stop_handler.gointernal/restapi/block_sequence_helper.go
3rabiii
force-pushed
the
fix/arrival-departure-endpoint
branch
from
944bd9e to
6ca0ece
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/restapi/arrival_and_departure_for_stop_handler.go (1)
823-846:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard the fallback path from unresolved block sequence values.
Line 845 can still compute
numberOfStopsAwaywhen either sequence is-1, which returns misleading values instead ofnil.Suggested fix
currentVehicleStopSequence := getCurrentVehicleStopSequence(vehicle) if currentVehicleStopSequence != nil { vehicleGlobalSeq := api.getBlockSequenceForStopSequence(ctx, activeTripID, *currentVehicleStopSequence, serviceDate) - numberOfStopsAway := targetGlobalSeq - vehicleGlobalSeq - return &numberOfStopsAway + if targetGlobalSeq != -1 && vehicleGlobalSeq != -1 { + numberOfStopsAway := targetGlobalSeq - vehicleGlobalSeq + return &numberOfStopsAway + } }🤖 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 `@internal/restapi/arrival_and_departure_for_stop_handler.go` around lines 823 - 846, In getNumberOfStopsAway, the fallback branch computes numberOfStopsAway even when getBlockSequenceForStopSequence returns -1 for targetGlobalSeq or vehicleGlobalSeq; update the fallback path in getNumberOfStopsAway to first check if targetGlobalSeq == -1 or vehicleGlobalSeq == -1 and return nil in that case, otherwise compute numberOfStopsAway = targetGlobalSeq - vehicleGlobalSeq and return its pointer; reference functions/values: getNumberOfStopsAway, getBlockSequenceForStopSequence, targetGlobalSeq, vehicleGlobalSeq, getCurrentVehicleStopSequence.
🤖 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.
Duplicate comments:
In `@internal/restapi/arrival_and_departure_for_stop_handler.go`:
- Around line 823-846: In getNumberOfStopsAway, the fallback branch computes
numberOfStopsAway even when getBlockSequenceForStopSequence returns -1 for
targetGlobalSeq or vehicleGlobalSeq; update the fallback path in
getNumberOfStopsAway to first check if targetGlobalSeq == -1 or vehicleGlobalSeq
== -1 and return nil in that case, otherwise compute numberOfStopsAway =
targetGlobalSeq - vehicleGlobalSeq and return its pointer; reference
functions/values: getNumberOfStopsAway, getBlockSequenceForStopSequence,
targetGlobalSeq, vehicleGlobalSeq, getCurrentVehicleStopSequence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aff60daf-bbe0-42c9-a9e0-d6dee3a95f73
📒 Files selected for processing (2)
internal/restapi/arrival_and_departure_for_stop_handler.gointernal/restapi/block_sequence_helper.go
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
3rabiii
force-pushed
the
fix/arrival-departure-endpoint
branch
from
6ca0ece to
de044be
Compare
|
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
This PR addresses several critical functional parity gaps in the
arrival-and-departure-for-stopAPI endpoint to ensure it matches the legacy OneBusAway implementation and strictly follows the API specification.Key Changes
Gap 1
vehicleId: Formats the vehicle ID into the combined{agencyId}_{vehicleId}structure instead of returning the raw GTFS ID.Gap 2: Sparse Sequence & Arrival/Departure Fix`: Computed a 0-based stop_ordinal in SQL to handle sparse GTFS sequences. This stable ordinal dynamically controls the arrival/departureEnabled flags and prevents 404 errors by safely matching client echo-requests.
Gap 3
routeShortName: Implements the 3-step narrative fallback for route short names.Gap 4
tripHeadsign: Implements the stop-level narrative override for trip headsigns.Gap 5
lastUpdateTime: Fixes the JSON serialization tag so it correctly emits0when there is no real-time vehicle data, instead of omitting the field entirely.Gap 6
status& Cancelled Trips: Properly handlesCANCELEDschedule relationships by setting the entrystatusto"CANCELED", zeroing out predictions, stripping the vehicle ID, and omitting thetripStatusobject.Gap 7 Dynamic
status: Maps non-scheduled GTFS-RT statuses (likeADDEDorDUPLICATED) directly to the top-level entrystatusinstead of unconditionally returning"default".Gap 8 Prediction Zeroing: Ensures
predictedArrivalTimeandpredictedDepartureTimeare reset to0whenpredictedisfalse, rather than erroneously falling back to the scheduled times.Gap 9 serviceDate Parsing: Supports YYYY-MM-DD date strings in the serviceDate parameter and correctly localizes dates to the agency's timezone to prevent calendar-day shifts.
Gap 10 time Parsing: Supports yyyy-MM-dd_HH-mm-ss datetime strings in the time parameter and localizes it to the agency's timezone.
Gap 11 includeReferences: Supports includeReferences=false for the singular endpoint to omit reference details (Agencies, Routes, Stops, Trips) from the response when requested.
Gap 12 stopSequence Search: Implements the expand-outward search algorithm for stopSequence matching, permitting tolerance to shifted indices from schedule modifications.
Gap 13 Closest Stop Visit: Resolves loop route ambiguity by selecting the closest scheduled stop visit using SQLite-driven absolute time difference calculations when stopSequence is absent.
Gap 14 blockTripSequence Fix: Accurately computes the 0-based trip sequence within a block by filtering active services. Updates the sentinel value to -1 for blockless trips and ensures the first trip (index 0) is correctly sent to the client.
Gap 15 Fix numberOfStopsAway: The numberOfStopsAway field in the arrival-and-departure-for-stop API was incorrectly returning 0 in Maglev. The previous implementation strictly relied on the GTFS-RT CurrentStopSequence to locate the vehicle, which is frequently unpopulated in many feeds
Missing Data Fallback: Updated getNumberOfStopsAway to accept the tripStatus object.
New Block Sequence Helper: Introduced getBlockSequenceForStopID in block_sequence_helper.go to seamlessly translate a raw stopID into its global block sequence ordinal.
Distance-Based Lookup Fallback: Refactored buildTripStatusCore in trips_helper.go to compute shape points and distances earlier.
Formula Correction: Fixed the calculation logic to use a direct ordinal subtraction (target - vehicle) instead of the previous off-by-one formula (target - vehicle - 1).
Testing: Added new test cases (TestGetNumberOfStopsAway_Valid and TestGetNumberOfStopsAway_UsesTripStatusNextStop) to verify the correct ordinal differences and ensure both data paths resolve accurately.
Summary by CodeRabbit
Bug Fixes & Improvements
Chores
Tests