Ramdom advice batch#640
Closed
OneLastChanc3 wants to merge 4 commits into
Closed
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 1 minute and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds a new ChangesAdvice Batch Endpoint
Sequence DiagramsequenceDiagram
participant Client
participant HTTPHandler as HTTP Handler
participant Service as Service
participant Database as Database
Client->>HTTPHandler: POST /advice/batch {"count":2}
HTTPHandler->>HTTPHandler: Validate count (1-50)
HTTPHandler->>Service: RandomBatch(ctx, 2)
Service->>Database: Query 1: random advice
Service->>Database: Query 2: random advice
Database-->>Service: Advice result 1
Database-->>Service: Advice result 2
Service-->>HTTPHandler: []Advice{...}
HTTPHandler->>HTTPHandler: Build BatchResponse
HTTPHandler-->>Client: HTTP 200 {"data":{"results":[...]}}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
UltiRequiem
requested changes
May 18, 2026
| httpx.JSON(w, http.StatusOK, a) | ||
| }) | ||
| r.Post("/advice/batch", func(w http.ResponseWriter, r *http.Request) { | ||
| var req BatchRequest |
Member
There was a problem hiding this comment.
Please use https://github.com/bobadilla-tech/requiems-api/blob/main/docs/core/batch-apis-rfc.md
that function handles the headers
| return | ||
| } | ||
|
|
||
| if req.Count <= 0 { |
Member
There was a problem hiding this comment.
Please use validation structs
…idation status codes
Member
|
@copilot resolve the merge conflicts in this pull request |
![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: 9
🧹 Nitpick comments (2)
apps/api/services/entertainment/advice/service.go (1)
66-83: ⚡ Quick winConsider fail-fast cancellation after first query error.
Right now all goroutines keep querying even after one scan failure is known. Canceling the shared context on first error will reduce wasted DB work under failure conditions.
Suggested pattern
go func(idx int) { defer wg.Done() @@ if err := row.Scan(&a.ID, &a.Text); err != nil { + cancel() // stop sibling queries ASAP errCh <- fmt.Errorf("scan advice: %w", err) return }🤖 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 `@apps/api/services/entertainment/advice/service.go` around lines 66 - 83, The goroutines keep running DB work after the first scan error; modify the function to create a cancellable context (context.WithCancel) and pass that ctx into the DB calls (e.g., QueryRowContext/row.Scan), and when sending an error into errCh call cancel() immediately so other workers see ctx.Done() and abort; ensure goroutines check ctx.Err()/select on ctx.Done() before/after starting queries to stop early, and keep the existing wg.Wait/errCh handling to collect the first error.apps/api/services/entertainment/advice/transport_http_test.go (1)
55-61: ⚡ Quick winAssert
totalandX-Usage-Countin the happy-path test.Since
HandleBatchderives both fromlen(results), adding these assertions will protect the endpoint’s response contract and usage-count billing behavior.🤖 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 `@apps/api/services/entertainment/advice/transport_http_test.go` around lines 55 - 61, Add assertions in the happy-path test that verify the response contract and billing usage count: assert that got.Data.Total equals 2 (matching len(got.Data.Results)) and assert the HTTP response header "X-Usage-Count" equals "2" (the same derived count from HandleBatch). Locate the test around the existing assertions on got.Data.Results in transport_http_test.go and add the two checks so the endpoint’s total and usage-count behavior are enforced.
🤖 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 `@apps/api/services/entertainment/advice/service_test.go`:
- Around line 173-185: The test's shared counter "call" inside the
mockRow.scanFn is mutated concurrently causing a data race in
TestRandomBatch_ReturnsPartialResultsOnFailure; fix it by replacing the plain
int counter with a concurrency-safe counter (e.g. sync/atomic or a mutex) and
update scanFn to read/increment via atomic operations (or lock/unlock) so that
the conditional (when counter == 3 return scanErr) and assignments to dest use
the thread-safe value; locate this in the test where newTestService is
constructed with mockRow.scanFn and update that closure to use atomic.AddInt32 /
atomic.LoadInt32 or a sync.Mutex-protected int.
In `@apps/api/services/entertainment/advice/service.go`:
- Around line 40-45: RandomBatch currently allocates results := make([]Advice,
count) which will panic if count < 0; add defensive validation at the start of
Service.RandomBatch (the method on type Service) to check the incoming count
parameter and return a clear error for negative values (e.g., validate count >=
0) and handle count==0 by returning an empty slice immediately; only allocate
results after the validation to avoid any negative-length slice panics.
In `@apps/api/services/entertainment/advice/transport_http.go`:
- Around line 28-41: The callback passed to httpx.HandleBatch currently returns
plain fmt.Errorf errors (for req.Count validations and when svc.RandomBatch
returns no data), which httpx treats as unexpected 500s; change those returns to
use typed errors the HTTP layer recognizes (e.g., a validation/client error type
for "count must be greater than zero" and "max 50 items allowed", and a
service-level NotFound/NoContent error for the no-advice case) instead of
fmt.Errorf so httpx.HandleBatch maps them to the correct HTTP status and error
code; update the error returns in the HandleBatch callback (the branch around
req.Count checks and the svc.RandomBatch error branch) to construct/return the
appropriate typed error values (and still wrap underlying errors with %w where
applicable).
In `@apps/api/services/entertainment/advice/type.go`:
- Line 10: This file fails gofmt on line 10; run the Go formatter on the file
(e.g., gofmt -w) to fix the whitespace/formatting issues in
apps/api/services/entertainment/advice/type.go and re-run the linter to ensure
the formatting error is resolved; no code changes beyond formatting are
required.
In `@apps/dashboard/config/api_docs/advice.yml`:
- Around line 126-128: The docs currently list the "service_unavailable" entry
with status: 503 for the "No advice available in database" case; update this
entry to match the transport-layer contract (use HTTP 500) by changing the
status value from 503 to 500 (or alternatively change the documented code to
match whatever the implementation uses), ensuring the YAML block containing
"code: service_unavailable" and "when: No advice available in database" reflects
the implemented HTTP 500 behavior.
- Line 82: Update the documentation for the batch parameter "count" to specify
the valid range "1..50" (instead of just >0) wherever it's described (the
"description" field for the count parameter) and update the related
invalid_count error documentation to state that invalid_count covers values
outside 1..50 (not just <=0); look for the "count" parameter description and the
"invalid_count" error entries in this advice.yml and change their text to
reflect the 1..50 constraint.
In `@docs/apis/entertainment/advice.md`:
- Line 127: Update the `count` parameter documentation and error wording to
state the valid range (1–50) instead of just “greater than zero”; specifically
change the table row that describes `count` (the table cell containing "`count`
| integer | ✅ | Number of advice items to return...") and any example/validation
error text that currently says only “greater than zero” so they explicitly read
something like “Must be between 1 and 50 (inclusive).” Ensure both the request
parameter description and the corresponding error message examples are revised
to mention the 1–50 limit.
- Around line 163-167: The docs table in advice.md incorrectly lists
`service_unavailable` as 503 while the endpoint's transport layer returns 500
for the "no advice available" case; update the table entry for
`service_unavailable` to use status 500 (in the row with the
`service_unavailable` code) so the documentation matches the actual transport
behavior, or alternatively update the endpoint implementation to return 503—pick
one consistent approach and ensure the `service_unavailable` row in the table
and any related description reflect that chosen status.
- Around line 236-243: The file contains duplicate top-level headings "## Use
Cases" and "## Features" which trigger MD024; rename or demote these repeated
headings to unique alternatives (for example change "## Use Cases" to "### Use
Cases" or "## Batch Use Cases" and change "## Features" to "### Features" or "##
Batch Features") so the heading hierarchy is unique and lint warnings are
resolved; update only the header lines shown (the headings themselves) and
adjust any nearby anchor references if needed.
---
Nitpick comments:
In `@apps/api/services/entertainment/advice/service.go`:
- Around line 66-83: The goroutines keep running DB work after the first scan
error; modify the function to create a cancellable context (context.WithCancel)
and pass that ctx into the DB calls (e.g., QueryRowContext/row.Scan), and when
sending an error into errCh call cancel() immediately so other workers see
ctx.Done() and abort; ensure goroutines check ctx.Err()/select on ctx.Done()
before/after starting queries to stop early, and keep the existing wg.Wait/errCh
handling to collect the first error.
In `@apps/api/services/entertainment/advice/transport_http_test.go`:
- Around line 55-61: Add assertions in the happy-path test that verify the
response contract and billing usage count: assert that got.Data.Total equals 2
(matching len(got.Data.Results)) and assert the HTTP response header
"X-Usage-Count" equals "2" (the same derived count from HandleBatch). Locate the
test around the existing assertions on got.Data.Results in
transport_http_test.go and add the two checks so the endpoint’s total and
usage-count behavior are enforced.
🪄 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: 8770428f-1459-4652-a1a7-ff5a810a1a8d
📒 Files selected for processing (8)
apps/api/services/entertainment/advice/service.goapps/api/services/entertainment/advice/service_test.goapps/api/services/entertainment/advice/transport_http.goapps/api/services/entertainment/advice/transport_http_test.goapps/api/services/entertainment/advice/type.goapps/dashboard/config/api_catalog.ymlapps/dashboard/config/api_docs/advice.ymldocs/apis/entertainment/advice.md
| - name: count | ||
| type: integer | ||
| required: true | ||
| description: Number of advice items to return. Must be greater than zero. |
Contributor
There was a problem hiding this comment.
Batch count constraints are incomplete in the request/error docs.
Line 82 and Line 123-Line 125 only describe count > 0, but the PR objective sets a maximum of 50. Please document count as 1..50 and reflect that in the invalid_count error condition.
Also applies to: 123-125
🤖 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 `@apps/dashboard/config/api_docs/advice.yml` at line 82, Update the
documentation for the batch parameter "count" to specify the valid range "1..50"
(instead of just >0) wherever it's described (the "description" field for the
count parameter) and update the related invalid_count error documentation to
state that invalid_count covers values outside 1..50 (not just <=0); look for
the "count" parameter description and the "invalid_count" error entries in this
advice.yml and change their text to reflect the 1..50 constraint.
|
|
||
| | Field | Type | Required | Description | | ||
| | ------- | ------- | -------- | ------------------------------------------------------------ | | ||
| | `count` | integer | ✅ | Number of advice items to return. Must be greater than zero. | |
Contributor
There was a problem hiding this comment.
count validation docs should include the max limit.
Line 127 and Line 166 currently imply only “greater than zero,” but this endpoint is constrained to 1–50. Please update both request and error wording accordingly.
Also applies to: 166-167
🤖 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 `@docs/apis/entertainment/advice.md` at line 127, Update the `count` parameter
documentation and error wording to state the valid range (1–50) instead of just
“greater than zero”; specifically change the table row that describes `count`
(the table cell containing "`count` | integer | ✅ | Number of advice items to
return...") and any example/validation error text that currently says only
“greater than zero” so they explicitly read something like “Must be between 1
and 50 (inclusive).” Ensure both the request parameter description and the
corresponding error message examples are revised to mention the 1–50 limit.
Comment on lines
+236
to
+243
| ## Use Cases | ||
|
|
||
| - **Daily Motivation Apps** - Provide users with daily wisdom and inspiration | ||
| - **Chatbot Responses** - Add helpful advice to conversational AI responses | ||
| - **Content Placeholders** - Fill content areas during development | ||
| - **Quote Widgets** - Display rotating advice on websites and dashboards | ||
|
|
||
| ## Features |
Contributor
There was a problem hiding this comment.
Duplicate top-level headings trigger markdown lint warnings.
Line 236 (## Use Cases) and Line 243 (## Features) duplicate earlier top-level headings, causing MD024 warnings. Rename or demote the repeated headings (e.g., “Batch Use Cases”, “Batch Features”) to keep heading hierarchy unique.
🧰 Tools
🪛 LanguageTool
[style] ~239-~239: Consider an alternative adjective to strengthen your wording.
Context: ...spiration - Chatbot Responses - Add helpful advice to conversational AI responses -...
(HELPFUL_CONSTRUCTIVE)
🪛 markdownlint-cli2 (0.22.1)
[warning] 236-236: Multiple headings with the same content
(MD024, no-duplicate-heading)
[warning] 243-243: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 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 `@docs/apis/entertainment/advice.md` around lines 236 - 243, The file contains
duplicate top-level headings "## Use Cases" and "## Features" which trigger
MD024; rename or demote these repeated headings to unique alternatives (for
example change "## Use Cases" to "### Use Cases" or "## Batch Use Cases" and
change "## Features" to "### Features" or "## Batch Features") so the heading
hierarchy is unique and lint warnings are resolved; update only the header lines
shown (the headings themselves) and adjust any nearby anchor references if
needed.
OneLastChanc3
force-pushed
the
ramdom_advice-batch
branch
from
May 26, 2026 09:18
e6e0854 to
1391afc
Compare
OneLastChanc3
force-pushed
the
ramdom_advice-batch
branch
from
May 26, 2026 09:42
1391afc to
4a84c4e
Compare
UltiRequiem
requested changes
May 26, 2026
| } | ||
|
|
||
|
|
||
| func (s *Service) RandomBatch(ctx context.Context, count int) ([]Advice, error) { |
Member
There was a problem hiding this comment.
This could actually be done with a single query instead of spawning multiple goroutines. For example, using ORDER BY random() LIMIT $1 would fetch all the rows at once and avoid the overhead of multiple DB calls.
func (s *Service) RandomBatch(ctx context.Context, count int) ([]Advice, error) {
ctx, cancel := context.WithTimeout(ctx, 3*time.Second)
defer cancel()
rows, err := s.db.QueryContext(ctx, `
SELECT id, text
FROM advice
ORDER BY random()
LIMIT $1;
`, count)
if err != nil {
return nil, err
}
defer rows.Close()
results := make([]Advice, 0, count)
for rows.Next() {
var a Advice
if err := rows.Scan(&a.ID, &a.Text); err != nil {
return nil, err
}
results = append(results, a)
}
if err := rows.Err(); err != nil {
return nil, err
}
return results, nil
}This will be a good read: https://www.namitjain.com/blog/n-plus-1-query-problem
| @@ -0,0 +1,18 @@ | |||
| package advice | |||
|
|
|||
Member
There was a problem hiding this comment.
UltiRequiem
added a commit
that referenced
this pull request
May 31, 2026
Batch random advice fetch for up to 50 items. Uses single SQL query ORDER BY random() LIMIT $1 per batch-apis.md rule 1 (no N-round-trips). Fixes from review: - Delete type.go; move Advice to service.go, BatchRequest to transport - Replace N-goroutine approach with single batch query - Follow exercises service pattern: local dbRows/dbPool interfaces + poolWrapper for testability without exposing *pgxpool.Pool - Remove manual count validation (struct tags handle it) - Remove IsData() methods (not supported) - Update service_test.go and transport_http_test.go for new interface Co-Authored-By: OneLastChanc3 <OneLastChanc3@users.noreply.github.com>
UltiRequiem
added a commit
that referenced
this pull request
May 31, 2026
Co-Authored-By: OneLastChanc3 <OneLastChanc3@users.noreply.github.com>
Member
Closed because it landed directly on main! 🚀
|
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.
-add POST /v1/entertainment/advice/batch endpoint
-add testing for service and transport_http
-add documentation
Summary by CodeRabbit
New Features
Documentation