Refactor HandleFindStop into smaller, tested functions#9
Open
aaronbrethorst wants to merge 1 commit into
Open
Conversation
Split the ~93-line HandleFindStop into four focused helpers (bindFindStopRequest, tryHandleDisambiguationChoice, resolveStopID, respondForStopID), reducing the handler to a 22-line pipeline. Replace the repeated max-9 DTMF clamp with a maxVoiceChoices const and clampChoiceCount helper. Add in-package characterization tests covering every branch; combined with the existing handlers-package tests, HandleFindStop is at 100% statement coverage and the extracted helpers are well covered. Behavior is unchanged. Closes #1
📝 WalkthroughWalkthrough
ChangesHandleFindStop Refactor and Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🧹 Nitpick comments (1)
handlers/voice/find_stop_test.go (1)
315-329: ⚡ Quick winStrengthen the lookup-error helper test with an output assertion.
This test only verifies that the mock was called. Add one observable response assertion (e.g., expected fallback/error prompt in body) so regressions in error rendering don’t slip through.
🤖 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 `@handlers/voice/find_stop_test.go` around lines 315 - 329, The test TestGetAndFormatVoiceArrivals_LookupError verifies that the mock client was called but does not assert anything about the actual response output. Add an assertion after the handler call to check that the response body (accessible via the httptest.ResponseRecorder w) contains the expected fallback or error prompt message. This ensures that when a lookup error occurs, the handler properly renders an error response to the user, not just that the mock was invoked.
🤖 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 `@handlers/voice/find_stop_test.go`:
- Around line 110-118: The test TestHandleFindStop_InvalidPhoneNumber currently
only verifies that the response body is non-empty but does not assert the HTTP
status code returned by the handler. Add an assertion to check the HTTP status
code on the response writer w to make the test deterministic and catch cases
where the handler might return an incorrect status while still having a
non-empty body. Reference the status code from the response writer object that
is populated by the postFindStop call.
In `@handlers/voice/find_stop.go`:
- Around line 287-289: The truncation message "Only showing first 9 options. "
at line 288 contains a hard-coded "9" that can drift from the maxVoiceChoices
constant if it is ever changed. Replace the literal "9" in this message string
with the actual maxVoiceChoices value using string formatting (e.g., sprintf or
similar formatting function) so the displayed number stays synchronized with the
constant, eliminating the magic-number coupling.
---
Nitpick comments:
In `@handlers/voice/find_stop_test.go`:
- Around line 315-329: The test TestGetAndFormatVoiceArrivals_LookupError
verifies that the mock client was called but does not assert anything about the
actual response output. Add an assertion after the handler call to check that
the response body (accessible via the httptest.ResponseRecorder w) contains the
expected fallback or error prompt message. This ensures that when a lookup error
occurs, the handler properly renders an error response to the user, not just
that the mock was invoked.
🪄 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: fa67a2c6-1816-4e0c-8ebb-0ab3969cfe40
📒 Files selected for processing (2)
handlers/voice/find_stop.gohandlers/voice/find_stop_test.go
Comment on lines
+110
to
+118
| func TestHandleFindStop_InvalidPhoneNumber(t *testing.T) { | ||
| r, mockClient, _ := setupFindStopHandler() | ||
|
|
||
| w := postFindStop(r, "abc", "12345") | ||
|
|
||
| // Phone validation fails before any OBA lookup happens. | ||
| mockClient.AssertNotCalled(t, "FindAllMatchingStops", mock.Anything) | ||
| assert.NotEqual(t, "", w.Body.String()) | ||
| } |
There was a problem hiding this comment.
Assert the HTTP status in this invalid-phone test.
Line 117 currently checks only non-empty body. This can pass even if the handler returns the wrong status code. Add a StatusOK assertion to keep this scenario deterministic.
Suggested patch
func TestHandleFindStop_InvalidPhoneNumber(t *testing.T) {
r, mockClient, _ := setupFindStopHandler()
w := postFindStop(r, "abc", "12345")
// Phone validation fails before any OBA lookup happens.
mockClient.AssertNotCalled(t, "FindAllMatchingStops", mock.Anything)
+ assert.Equal(t, http.StatusOK, w.Code)
assert.NotEqual(t, "", w.Body.String())
}📝 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
| func TestHandleFindStop_InvalidPhoneNumber(t *testing.T) { | |
| r, mockClient, _ := setupFindStopHandler() | |
| w := postFindStop(r, "abc", "12345") | |
| // Phone validation fails before any OBA lookup happens. | |
| mockClient.AssertNotCalled(t, "FindAllMatchingStops", mock.Anything) | |
| assert.NotEqual(t, "", w.Body.String()) | |
| } | |
| func TestHandleFindStop_InvalidPhoneNumber(t *testing.T) { | |
| r, mockClient, _ := setupFindStopHandler() | |
| w := postFindStop(r, "abc", "12345") | |
| // Phone validation fails before any OBA lookup happens. | |
| mockClient.AssertNotCalled(t, "FindAllMatchingStops", mock.Anything) | |
| assert.Equal(t, http.StatusOK, w.Code) | |
| assert.NotEqual(t, "", w.Body.String()) | |
| } |
🤖 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 `@handlers/voice/find_stop_test.go` around lines 110 - 118, The test
TestHandleFindStop_InvalidPhoneNumber currently only verifies that the response
body is non-empty but does not assert the HTTP status code returned by the
handler. Add an assertion to check the HTTP status code on the response writer w
to make the test deterministic and catch cases where the handler might return an
incorrect status while still having a non-empty body. Reference the status code
from the response writer object that is populated by the postFindStop call.
Comment on lines
+287
to
289
| if len(stops) > maxVoiceChoices { | ||
| msg += "Only showing first 9 options. " | ||
| } |
There was a problem hiding this comment.
Replace the remaining hard-coded 9 in truncation messaging.
Line 288 still hard-codes 9 ("Only showing first 9 options. "), which can drift from maxVoiceChoices if the constant changes. Please format this message from maxVoiceChoices (or localized string args) so the refactor fully removes magic-number coupling.
🤖 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 `@handlers/voice/find_stop.go` around lines 287 - 289, The truncation message
"Only showing first 9 options. " at line 288 contains a hard-coded "9" that can
drift from the maxVoiceChoices constant if it is ever changed. Replace the
literal "9" in this message string with the actual maxVoiceChoices value using
string formatting (e.g., sprintf or similar formatting function) so the
displayed number stays synchronized with the constant, eliminating the
magic-number coupling.
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.
Closes #1
Summary
HandleFindStop(handlers/voice/find_stop.go) into four focused helpers —bindFindStopRequest,tryHandleDisambiguationChoice,resolveStopID,respondForStopID— reducing the handler itself to a 22-line top-to-bottom pipeline: bind → maybe-handle-disambiguation → resolve stop ID → respond.9DTMF ceiling (open-coded in 4 places) with amaxVoiceChoicesconst and aclampChoiceCounthelper.handlers/voice/find_stop_test.go, packagevoice) covering every branch. Combined with the existinghandlers-package tests,HandleFindStopis at 100% statement coverage and the extracted helpers are well covered (80–100%).Test plan
make fmt/make vet/make lint(0 issues) /make testall passReview notes (non-blocking follow-ups, intentionally out of scope here)
bindFindStopRequestoverlaps with the existingpreprocessRequesthelper, but they are not interchangeable:preprocessRequestalso fires analytics (TrackVoiceRequest), which the originalHandleFindStopnever did. Unifying them is a deliberate behavior decision (should find-stop calls be tracked?) — left for a follow-up to keep this PR behavior-preserving.handleVoiceDisambiguationChoicestill re-fetches the session and re-validates the choice range thattryHandleDisambiguationChoicealready checked; that downstream range check is now effectively unreachable on the real path (a pre-existing redundancy, not introduced here). Could be collapsed by passing the session through.voice-package tests overlap with severalHandleFindStopcases inhandlers/voice_menu_test.go, which could be retired in favor of these.Summary by CodeRabbit
Bug Fixes
Tests