docs: concepts glossary, streaming RPCs, and database/cache/messaging how-tos#87
Merged
Merged
Conversation
Five new pages plus cross-links and matching test coverage.
concepts.md (top-level, /concepts)
- One-paragraph definitions for the gRPC, observability, and resilience
terms that show up throughout the docs: gRPC, gRPC reflection,
grpc-gateway, interceptors, vtprotobuf, Protovalidate, OTLP, trace ID,
span, circuit breaker, healthcheck vs readycheck, lifecycle hooks.
- Each entry links into the deeper how-to or external doc, so the page
drops the "you must already know gRPC" prerequisite without
duplicating canonical content.
- Linked from Index.md "Next Steps".
howto/streaming-rpcs.md
- Server-streaming, client-streaming, and bidirectional handler patterns
with proto + Go examples.
- Deadline propagation through stream contexts and the cancel-on-Done
pattern for long-lived streams.
- Backpressure: gRPC-Go uses Send blocking as the implicit signal —
documents the producer-loop, bounded-buffer, and don't-fan-out-to-
goroutines patterns explicitly.
- grpc-gateway HTTP-streaming limits: NDJSON for server-streaming,
limited support for client/bidi over HTTP/1.1, the X-Accel-Buffering
reverse-proxy gotcha, and runtime.WithStreamErrorHandler.
- The FAQ entry shrinks to a one-line pointer at this page.
howto/database.md
- Lifecycle pattern: pool init in PreStart, drain in Stop,
NewDatastoreSpan around each query.
- Concrete Postgres example with pgx/pgxpool covering transactions,
error handling, and pool sizing relative to gRPC concurrency.
- Library-agnostic: explicit pointers for MySQL, CockroachDB, MongoDB,
Spanner so readers know the same pattern works for any client.
howto/cache.md
- Same lifecycle pattern with Redis / Valkey via go-redis as the worked
example. Cache-aside with the "cache failures must not fail the
request" principle, TTL guidance, write-through invalidation.
- Pointers for Memcached, in-process LRU, and multi-tier caches.
howto/messaging.md
- Consumers run as workers via CBWorkerProvider + workers.NewWorker.
Long-running pattern: block on ctx.Done() inside the handler so
graceful drain happens automatically when core cancels worker context.
- Kafka example with segmentio/kafka-go (context-aware reader maps
cleanly onto the worker shape).
- NATS example with nats-io/nats.go (callback subscription; PreStart
opens conn, Stop calls Drain, worker holds subscription open).
- Cross-broker tracing options (header propagation vs domain-ID linking)
and the at-least-once / make-handlers-idempotent reality.
Configuration pattern
- Every database/cache/messaging example shows extending the cookiecutter
Config struct in config/config.go with envconfig tags and accessing
values via config.Get().FieldName, matching the cookiecutter's own
config/config.go (which embeds cbConfig.Config). Avoids raw os.Getenv,
which is unvalidated, untyped, and silently tolerates missing values.
- Import path placeholder is "myapp/config" with a comment.
Cross-links
- howto/local-dev.md gets a one-line pointer to the database, cache, and
messaging how-tos next to the matching profile tables.
- howto/signals.md step 4 ("Stop workers") points at the messaging
how-to for the consumer-drain pattern.
Tests
- Six new content assertions in tests/content.spec.ts cover the new
pages: required headings on /concepts/, key sections on
/howto/streaming-rpcs/, and config.Get()-based examples on the three
data how-tos.
- A `newHowtoPages` constant declared once at the top of
tests/content.spec.ts is reused by the SEO meta-description and
Table-of-Contents sets so adding a how-to only touches one list.
- /concepts/ added to topLevelPages, SEO, and TOC sets.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a Concepts glossary and four How‑To guides (Streaming RPCs, Database, Cache, Messaging), updates site navigation and the streaming FAQ, and extends Playwright tests to include the new pages and routing. All edits are documentation and test changes. ChangesDocumentation & Test Additions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 54 minutes and 18 seconds.Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new Concepts glossary and several new how-to guides to make ColdBrew’s docs more approachable (especially for readers new to gRPC/streaming and long-lived dependencies), with Playwright updates to keep navigation/SEO/TOC/content assertions in sync.
Changes:
- Add
/conceptsglossary page and link it from the home page. - Add new how-to pages for streaming RPCs, database, cache, and messaging patterns (with cross-links from existing docs).
- Extend Playwright navigation/content/SEO/TOC coverage for the new pages.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/navigation.spec.ts | Adds /concepts/ and new how-to routes to page-load coverage. |
| tests/content.spec.ts | Adds new content assertions; reuses a shared newHowtoPages list in SEO/TOC tests. |
| concepts.md | New glossary-style Concepts page with cross-links into deeper guides. |
| Index.md | Adds a “Concepts” next-step link on the home page. |
| FAQ.md | Replaces the streaming FAQ content with a pointer to the new Streaming RPCs how-to. |
| howto/index.md | Adds the new how-to entries to the Build table. |
| howto/local-dev.md | Adds links to the new database/cache/messaging how-tos near the profile tables. |
| howto/signals.md | Adds a link to the Messaging how-to from the shutdown “Stop workers” step. |
| howto/streaming-rpcs.md | New guide covering streaming shapes, patterns, deadlines, backpressure, and gateway limits. |
| howto/database.md | New guide documenting DB lifecycle hooks + pgx example + tracing guidance. |
| howto/cache.md | New guide documenting cache lifecycle hooks + go-redis example + cache-aside guidance. |
| howto/messaging.md | New guide documenting worker-based consumers with Kafka (kafka-go) and NATS (nats.go). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
concepts.md (1)
20-66: ⚡ Quick winConcepts page has no entry or link for Streaming RPCs.
Every other concept in this page that has a dedicated how-to page links to it. The PR adds
/howto/streaming-rpcs/but the glossary contains no "Streaming RPCs" entry, and the Lifecycle hooks section (the closest conceptual anchor for the data how-tos) doesn't link to/howto/database,/howto/cache, or/howto/messagingeither. Readers landing on/concepts/from the navigation have no path to those four new pages from this glossary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@concepts.md` around lines 20 - 66, Add a new "Streaming RPCs" concept entry in concepts.md (near other gRPC-related entries) that briefly defines streaming RPCs and links to /howto/streaming-rpcs/, and update the "Lifecycle hooks" paragraph in concepts.md to include links to the related how-tos (/howto/database, /howto/cache, /howto/messaging) so readers can navigate to those pages; edit the "Streaming RPCs" header and the "Lifecycle hooks" paragraph in concepts.md to add the appropriate inline links.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@concepts.md`:
- Line 6: The permalink in concepts.md currently is "permalink: /concepts" which
generates concepts.html and causes 404s for tests expecting a trailing slash;
update the permalink to include a trailing slash (e.g., change the permalink
value to "/concepts/") so Jekyll generates the directory-based path (index.html
under /concepts/) and aligns with navigation.spec.ts and content.spec.ts
expectations.
In `@howto/cache.md`:
- Around line 73-119: The code block for wiring the Redis client imports
"github.com/go-coldbrew/tracing" but never uses it, causing a Go compile error;
fix by removing the unused tracing import from the import list in this snippet
(or alternatively add a minimal use of tracing such as passing a tracer to a
function, but preferred is to simply delete the unused import) so that
Service.PreStart and Service.Stop compile cleanly without the "imported and not
used" error.
In `@howto/database.md`:
- Around line 24-28: The fenced code block containing the lifecycle notes
("PreStart", "Stop", "NewDatastoreSpan") should include a language identifier
(use "text") or be converted to a plain Markdown list; update the block around
the lines with the symbols PreStart, Stop, and NewDatastoreSpan to either start
with ```text or replace the triple-backtick block with a normal bulleted list so
the markdown linter accepts it.
In `@howto/messaging.md`:
- Around line 26-33: The fenced code block describing the worker pattern in
howto/messaging.md should include a language identifier to satisfy markdown
linters; update the block that documents CBWorkerProvider.Workers() (the block
that starts with "CBWorkerProvider.Workers() returns one *workers.Worker per
subscription.") by changing the opening fence from ``` to ```text so the snippet
is marked as plain text.
---
Nitpick comments:
In `@concepts.md`:
- Around line 20-66: Add a new "Streaming RPCs" concept entry in concepts.md
(near other gRPC-related entries) that briefly defines streaming RPCs and links
to /howto/streaming-rpcs/, and update the "Lifecycle hooks" paragraph in
concepts.md to include links to the related how-tos (/howto/database,
/howto/cache, /howto/messaging) so readers can navigate to those pages; edit the
"Streaming RPCs" header and the "Lifecycle hooks" paragraph in concepts.md to
add the appropriate inline links.
🪄 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: dfd05074-215b-48a4-a595-16b308fb3e91
📒 Files selected for processing (12)
FAQ.mdIndex.mdconcepts.mdhowto/cache.mdhowto/database.mdhowto/index.mdhowto/local-dev.mdhowto/messaging.mdhowto/signals.mdhowto/streaming-rpcs.mdtests/content.spec.tstests/navigation.spec.ts
…, NATS via ChannelWorker
concepts.md
- permalink is now /concepts/ (with trailing slash) so the page renders at
the URL the navigation and content tests already expect.
howto/database.md
- Postgres host port corrected from 5432 -> 5433 in the local-stack prose,
the DATABASE_URL example, and the Local stack profiles table; AlloyDB
Omni from 5432 -> 5434. Matches the cookiecutter docker-compose.local.yml
and the Local Development page.
howto/cache.md
- Valkey host port corrected from 6379 -> 6380. The "both bind 6379, so
they conflict" sentence was wrong on top of being misleading; now reads
as a side-by-side note pointing at /howto/local-dev#cache.
- Dropped the unused tracing import from the Wire-the-client snippet.
howto/streaming-rpcs.md
- Bidi example: the read on the incoming channel now uses the `, ok` form.
After the reader goroutine closes the channel on Recv error, the previous
code would have called s.handle(nil); the new code drops the case (sets
the channel to nil) and lets the next iteration pick up the error from
errCh, which was always the intended path.
howto/messaging.md (Kafka)
- Switched from segmentio/kafka-go to IBM/sarama for at-least-once
semantics. The previous kafka-go ReadMessage call auto-commits offsets
on read, which is at-most-once and silently drops messages that crash
mid-handle. The sarama consumer-group API (NewConsumerGroup +
ConsumerGroupHandler) marks offsets only after the handler succeeds and
flushes them on the auto-commit interval (default 1s), giving the
at-least-once guarantee the surrounding prose was already claiming.
- The outer for { Consume; ... } loop handles rebalances; sess.Context().
Done() is the cancellation point.
howto/messaging.md (NATS)
- Switched from QueueSubscribe (callback) to ChanQueueSubscribe + workers.
ChannelWorker. ChannelWorker already implements the select { ctx.Done() /
msgCh } loop, error propagation, and clean drain on cancellation, so the
service code only writes the handler. Added a brief callout naming the
win: handleOrder takes only (ctx, info, *nats.Msg), the channel is the
seam, and you can swap the source (mock, internal pipeline, different
broker) without touching the consumer.
- Pointer to workers.BatchChannelWorker for size/time-flushed batches.
tests/content.spec.ts
- Streaming-RPCs test name dropped the "all four shapes" claim; the
assertions only cover the three streaming shapes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
howto/messaging.md (1)
26-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language identifier to the “messaging consumer” fenced code block (MD040).
Same markdownlint issue as before: the pattern fence under “A messaging consumer is a long-running worker:” is currently untyped (no
text/goidentifier), which triggersMD040 (fenced-code-language).🛠️ Proposed fix
-``` +```text CBWorkerProvider.Workers() returns one *workers.Worker per subscription. Each worker: - opens its broker client - reads in a loop, handling each message - returns when ctx.Done() fires - returns ctx.Err() so core treats it as a clean shutdown</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@howto/messaging.mdaround lines 26 - 33, The fenced code block describing
the messaging consumer should include a language identifier to satisfy
markdownlint MD040; update the block that starts with
"CBWorkerProvider.Workers() returns one *workers.Worker per subscription." by
adding a language tag (e.g., use "text" or "go") after the opening triple
backticks so the block becomestext (orgo) and keep the existing lines
unchanged; locate the block near the "A messaging consumer is a long-running
worker:" section and modify that specific fenced code block only.</details> </blockquote></details> <details> <summary>howto/database.md (1)</summary><blockquote> `24-28`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add a language identifier to the “The pattern” fenced code block (MD040).** This untyped fenced block (`PreStart → ...`, `Stop → ...`, `NewDatastoreSpan ...`) will trip markdownlint `MD040 (fenced-code-language)`. Mark it as `text`. <details> <summary>🛠️ Proposed fix</summary> ```diff -``` +```text PreStart → open the pool, run a ping Stop → close the pool NewDatastoreSpan around each query → tracing + metrics ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@howto/database.mdaround lines 24 - 28, The fenced code block containing the
pattern lines "PreStart → open the pool, run a ping", "Stop → close the
pool", and "NewDatastoreSpan around each query → tracing + metrics" is missing a
language identifier and triggers markdownlint MD040; update that fenced block to
include the language tagtext(i.e., change the openingtotext) so the
block is properly typed while leaving the content (PreStart, Stop,
NewDatastoreSpan) unchanged.</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>howto/streaming-rpcs.md (4)</summary><blockquote> `105-149`: _⚡ Quick win_ **Bidi example: harden error signaling + optionally guard against nil messages.** In the bidi handler (lines ~105-149), the reader goroutine does `errCh <- err` directly (line ~116). Even though `errCh` is buffered to 1 and you only send once on the first `Recv` error, it’s still easy for readers to copy this pattern and accidentally create blocking edge cases later. Suggested tweaks (keeps the docs safe to replicate): - Make the error send non-blocking: - `select { case errCh <- err: default: }` - Optionally ignore nil messages defensively before calling `s.handle(msg)` (if you want the example to be extra robust): `if msg == nil { continue }`. <details> <summary>Proposed patch</summary> ```diff @@ go func() { defer close(incoming) for { msg, err := stream.Recv() if err != nil { - errCh <- err + select { + case errCh <- err: + default: + } return } select { case incoming <- msg: case <-ctx.Done(): return } } }() @@ - case msg, ok := <-incoming: + case msg, ok := <-incoming: if !ok { @@ - reply := s.handle(msg) + if msg == nil { + continue + } + reply := s.handle(msg) if err := stream.Send(reply); err != nil { return err } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@howto/streaming-rpcs.md` around lines 105 - 149, The bidi Chat handler should avoid potentially blocking on sending errors from the reader goroutine and optionally guard against nil messages before handling; update the goroutine that sends errors into errCh (in ChatService.Chat / reader goroutine) to use a non-blocking send (e.g., select { case errCh <- err: default: }) so it never blocks if nobody is receiving, and in the main loop before calling s.handle(msg) add a defensive nil check (if msg == nil { continue }) so we don't pass a nil message into s.handle. ``` </details> --- `154-167`: _💤 Low value_ **Deadline section: reword “only signal” slightly to avoid overclaiming.** The guidance at line ~164 says `stream.Context().Err()` is “your only signal to stop producing.” Since the text already says you can also “rely on `Send`/`Recv` returning”, the “only signal” phrasing is slightly absolute. Recommend a small wording change, e.g. “one of the key signals” / “most reliable signal” and explicitly mention stopping on `<-stream.Context().Done()` in producers as well. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@howto/streaming-rpcs.md` around lines 154 - 167, Reword the absolute phrasing around stream.Context().Err() so it doesn't claim it's the "only signal": change the sentence referencing stream.Context().Err() and Send/Recv to say it's "one of the key signals" or "the most reliable signal" to stop producing, and add a short note that producers should also select on <-stream.Context().Done() (or check stream.Context().Err()) inside long-running loops to exit promptly; update the sentence that currently mentions "your only signal to stop producing" to this revised wording and include the explicit mention of <-stream.Context().Done() and Send/Recv returning as alternative/related signals. ``` </details> --- `197-203`: _⚡ Quick win_ **Related links: double-check slug casing (notably `/howto/Tracing`).** The related link at line ~202 is `/howto/Tracing` (capital T). Many Jekyll doc repos treat slugs/casing inconsistently across tooling, and broken-link tests often fail on case mismatches. Please confirm the actual route is exactly `/howto/Tracing` (not `/howto/tracing`), and that the link targets are reachable in the Playwright navigation assertions. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@howto/streaming-rpcs.md` around lines 197 - 203, The Related section contains a possibly mismatched slug `/howto/Tracing`; verify the actual page slug (e.g., `/howto/Tracing` vs `/howto/tracing`) and update this link to exactly match the target route used by the site (or vice‑versa) so Playwright navigation assertions and broken-link tests pass; specifically check and correct the string `/howto/Tracing` in howto/streaming-rpcs.md and ensure the target doc filename/front-matter permalinks use the same casing, then re-run the link checker/Playwright tests. ``` </details> --- `168-177`: _💤 Low value_ **Backpressure section: OK, but consider one sentence clarifying single-goroutine Send ordering.** The bullets are good. The “Don’t fan-out…” bullet (line ~174) is the right core idea—maybe just ensure it explicitly ties to ordering + stream safety (“only one goroutine should call `Send` for a given stream, to preserve ordering and avoid races”). This is doc-only, but it helps prevent common misuse patterns. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@howto/streaming-rpcs.md` around lines 168 - 177, Update the "Don't fan-out into goroutines per message" bullet to explicitly state that only one goroutine should call stream.Send for a given stream to preserve message ordering and avoid races; reference the stream.Send call and ordering/stream-safety so readers understand they must serialize sends (no concurrent goroutines invoking stream.Send) or use a single sender loop/channel to multiplex safely. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@howto/cache.md:
- Around line 24-28: Add a language identifier to the fenced code block that
contains the lines "PreStart → open the client, run a Ping", "Stop → close
the client", and "NewDatastoreSpan around each call → tracing" by changing the
opening fence to use "text" (i.e., ```text) so markdownlint MD040 is satisfied;
keep the block content unchanged and only add the language tag.
Duplicate comments:
In@howto/database.md:
- Around line 24-28: The fenced code block containing the pattern lines
"PreStart → open the pool, run a ping", "Stop → close the pool", and
"NewDatastoreSpan around each query → tracing + metrics" is missing a language
identifier and triggers markdownlint MD040; update that fenced block to include
the language tagtext(i.e., change the openingtotext) so the block
is properly typed while leaving the content (PreStart, Stop, NewDatastoreSpan)
unchanged.In
@howto/messaging.md:
- Around line 26-33: The fenced code block describing the messaging consumer
should include a language identifier to satisfy markdownlint MD040; update the
block that starts with "CBWorkerProvider.Workers() returns one *workers.Worker
per subscription." by adding a language tag (e.g., use "text" or "go") after the
opening triple backticks so the block becomestext (orgo) and keep the
existing lines unchanged; locate the block near the "A messaging consumer is a
long-running worker:" section and modify that specific fenced code block only.
Nitpick comments:
In@howto/streaming-rpcs.md:
- Around line 105-149: The bidi Chat handler should avoid potentially blocking
on sending errors from the reader goroutine and optionally guard against nil
messages before handling; update the goroutine that sends errors into errCh (in
ChatService.Chat / reader goroutine) to use a non-blocking send (e.g., select {
case errCh <- err: default: }) so it never blocks if nobody is receiving, and in
the main loop before calling s.handle(msg) add a defensive nil check (if msg ==
nil { continue }) so we don't pass a nil message into s.handle.- Around line 154-167: Reword the absolute phrasing around
stream.Context().Err() so it doesn't claim it's the "only signal": change the
sentence referencing stream.Context().Err() and Send/Recv to say it's "one of
the key signals" or "the most reliable signal" to stop producing, and add a
short note that producers should also select on <-stream.Context().Done() (or
check stream.Context().Err()) inside long-running loops to exit promptly; update
the sentence that currently mentions "your only signal to stop producing" to
this revised wording and include the explicit mention of
<-stream.Context().Done() and Send/Recv returning as alternative/related
signals.- Around line 197-203: The Related section contains a possibly mismatched slug
/howto/Tracing; verify the actual page slug (e.g.,/howto/Tracingvs
/howto/tracing) and update this link to exactly match the target route used by
the site (or vice‑versa) so Playwright navigation assertions and broken-link
tests pass; specifically check and correct the string/howto/Tracingin
howto/streaming-rpcs.md and ensure the target doc filename/front-matter
permalinks use the same casing, then re-run the link checker/Playwright tests.- Around line 168-177: Update the "Don't fan-out into goroutines per message"
bullet to explicitly state that only one goroutine should call stream.Send for a
given stream to preserve message ordering and avoid races; reference the
stream.Send call and ordering/stream-safety so readers understand they must
serialize sends (no concurrent goroutines invoking stream.Send) or use a single
sender loop/channel to multiplex safely.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `8b6232ad-ecae-4c7f-8c41-1c9c3d52af20` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 0ba185484578b6df824b74fffe60340f06644a54 and 1f7bb525a510cb405f68bda53e35496ee2f7f4de. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `concepts.md` * `howto/cache.md` * `howto/database.md` * `howto/messaging.md` * `howto/streaming-rpcs.md` * `tests/content.spec.ts` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * concepts.md * tests/content.spec.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… pages list
- howto/{database,cache,messaging}.md: tagged the three pseudocode pattern
blocks as `text`. Renders identically in just-the-docs but satisfies
markdownlint MD040 and helps tooling announce them as code regions.
- howto/streaming-rpcs.md: bidi snippet uses io.EOF — added the missing
`import "io"` so the example compiles as written, matching the
client-streaming snippet above it.
- howto/messaging.md: fixed a connection leak in the NATS PreStart. If
ChanQueueSubscribe fails after Connect succeeds, we now Close the conn
and return; s.nc / s.msgCh are only assigned after the whole startup
succeeds, so a failure path leaves the service unchanged.
- tests/pages.ts: extracted the newHowtoPages list to a shared file.
content.spec.ts (SEO + TOC sets) and links.spec.ts (broken-link crawl)
both import it now, and links.spec.ts also crawls /concepts/. Adding a
new how-to is one edit instead of three.
Replaces the partial sampling lists with the full how-to set. Previously SEO checked ~16 of 24 how-tos, the TOC test checked ~14 of 24, and the broken-link crawl visited ~9 of 24. Pages like /howto/Log/, /howto/ Tracing/, /howto/swagger/, /howto/Metrics/, /howto/data-builder/, /howto/ vtproto/, /howto/testing/, /howto/private-modules/, /howto/Debugging/, and /howto/signals/ were missing from one or more of those checks; they are now all covered. tests/pages.ts - Renamed `newHowtoPages` -> `allHowtoPages`. The "new" naming would have rotted within a few PRs, and the all-pages set is what every test actually wants to walk anyway. tests/navigation.spec.ts, tests/content.spec.ts, tests/links.spec.ts - All three now import `allHowtoPages` and use it directly. content.spec. ts also extracts the top-level page set into a local `topLevelPages` const so SEO can spread both sets together. - pagesToCrawl in links.spec.ts now reflects the full top-level page set (previously missing /faq/) plus every how-to. Adds ~30-60s of test wall-clock for the broader coverage; flagged broken links / missing meta descriptions on previously-untested pages will fail the build instead of being silently shipped.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…PreStop tests/pages.ts - Reordered the `allHowtoPages` list to actually match nav_order. The comment claimed sidebar order but auth (19) was listed before readiness (17) and local-dev (18). Inline nav_order numbers as line comments so the next person can verify by inspection. howto/messaging.md (NATS) - Moved sub.Drain() from Stop() to a new PreStop() implementation. The shutdown sequence is: PreStop (step 1), drain wait (step 3), worker context cancellation (step 4), Stop (step 9). With Drain() in Stop(), the worker has already exited and there is no one reading from msgCh — Drain blocks on an unread channel until the connection is force- closed. Draining in PreStop instead means the broker stops pushing while the worker is still consuming, so the channel empties cleanly during the drain wait. - Service struct now holds *nats.Subscription so PreStop can reach it. - CBPreStopper interface assertion added. - Replaced the "Stop() calls Drain()" bullet with a "Drain order matters" bullet explaining why PreStop is the right hook here.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@howto/cache.md`:
- Line 184: Edit the sentence "the data you needed evicted first is rarely the
data Redis evicts first" in the howto/cache.md line to use standard grammar —
replace "needed evicted" with "need to evict" (e.g., "the data you need to evict
first is rarely the data Redis evicts first") so the phrasing is clear and
nonstandard double-modality is removed.
🪄 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: 76e534de-241e-48cd-b0ab-aceda693de3a
📒 Files selected for processing (8)
howto/cache.mdhowto/database.mdhowto/messaging.mdhowto/streaming-rpcs.mdtests/content.spec.tstests/links.spec.tstests/navigation.spec.tstests/pages.ts
✅ Files skipped from review due to trivial changes (1)
- tests/pages.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/navigation.spec.ts
- howto/messaging.md
| Two principles to keep in mind: | ||
|
|
||
| - **Cache failures must not fail the request.** A degraded cache should turn into higher database load, not 5xx errors. | ||
| - **Pick a TTL up front, not by accident.** `SET key value EX 300` (5 minutes) for human-scale data; longer for immutable data; explicit `Del` on writes for anything that *must* invalidate. Avoid relying on memory pressure for eviction — the data you needed evicted first is rarely the data Redis evicts first. |
There was a problem hiding this comment.
Nonstandard grammar: "needed evicted" → "need to evict".
The phrase "the data you needed evicted first is rarely the data Redis evicts first" uses a double modal construction that is grammatically nonstandard. This can trip up non-native English readers.
✏️ Proposed fix
-— the data you needed evicted first is rarely the data Redis evicts first.
+— the data you need to evict first is rarely the data Redis evicts first.📝 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
| - **Pick a TTL up front, not by accident.** `SET key value EX 300` (5 minutes) for human-scale data; longer for immutable data; explicit `Del` on writes for anything that *must* invalidate. Avoid relying on memory pressure for eviction — the data you needed evicted first is rarely the data Redis evicts first. | |
| - **Pick a TTL up front, not by accident.** `SET key value EX 300` (5 minutes) for human-scale data; longer for immutable data; explicit `Del` on writes for anything that *must* invalidate. Avoid relying on memory pressure for eviction — the data you need to evict first is rarely the data Redis evicts first. |
🧰 Tools
🪛 LanguageTool
[style] ~184-~184: ‘by accident’ might be wordy. Consider a shorter alternative.
Context: ...xx errors. - Pick a TTL up front, not by accident. SET key value EX 300 (5 minutes) f...
(EN_WORDINESS_PREMIUM_BY_ACCIDENT)
[style] ~184-~184: The double modal “needed evicted” is nonstandard (only accepted in certain dialects). Consider “to be evicted”.
Context: ...sure for eviction — the data you needed evicted first is rarely the data Redis evicts f...
(NEEDS_FIXED)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@howto/cache.md` at line 184, Edit the sentence "the data you needed evicted
first is rarely the data Redis evicts first" in the howto/cache.md line to use
standard grammar — replace "needed evicted" with "need to evict" (e.g., "the
data you need to evict first is rarely the data Redis evicts first") so the
phrasing is clear and nonstandard double-modality is removed.
Member
Author
There was a problem hiding this comment.
Tightened in the next push — went with "need evicted" (same passive construction, drops the awkward double-modal). Functionally equivalent to the suggested rewording but keeps the original sentence shape.
There was a problem hiding this comment.
@ankurs, acknowledged — fixing the tense (needed → need) removes the most confusing part of the original phrasing. Worth noting that "need evicted" (without "to be") is still flagged by LanguageTool under the same NEEDS_FIXED dialectal rule, so automated linting may surface it again. If that becomes noisy, "need to be evicted" is the fully standard passive form. Happy to leave it as-is if you prefer the shorter shape — just flagging it for awareness.
🧠 Learnings used
Learnt from: ankurs
Repo: go-coldbrew/docs.coldbrew.cloud PR: 41
File: Cookiecutter.md:0-0
Timestamp: 2026-03-26T14:26:19.053Z
Learning: For Jekyll/GitHub Pages redirect pages in this repo (docs.coldbrew.cloud), do not rely on a `jekyll-redirect-from` plugin. Instead: set `layout: null` in the page front matter so Jekyll emits a bare HTML file, add `<meta http-equiv="refresh" content="0; url=TARGET_URL">` in the document `<head>`, and include a client-side fallback in the `<body>` such as `window.location.replace('TARGET_URL')` to ensure redirects still work even if the meta refresh is ignored.
Learnt from: ankurs
Repo: go-coldbrew/docs.coldbrew.cloud PR: 75
File: integrations.md:242-270
Timestamp: 2026-04-17T12:36:15.989Z
Learning: When reviewing this docs.coldbrew.cloud repository, do not flag documentation as referencing “missing” or unreleased interceptor APIs if they exist only in a companion/unmerged PR (e.g., go-coldbrew/interceptors#45) and have not yet been released/tagged (e.g., not present in go-coldbrew/interceptors v0.1.9). For any referenced API that appears absent from the latest tagged package, verify the companion PR first; only raise an issue if the API is not present in the companion PR either.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
"needed evicted" -> "need evicted" — same passive construction, less awkward to read.
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
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.
Summary
Five new pages plus cross-links and matching test coverage:
/concepts— top-level glossary. One paragraph per term (gRPC, interceptors, vtprotobuf, OTLP, trace ID, span, lifecycle hooks, healthcheck vs readycheck, …) with a link into the deeper how-to. Drops the "you must already know gRPC" prerequisite without duplicating canonical content./howto/streaming-rpcs— server-streaming, client-streaming, and bidi handler patterns; deadline propagation; backpressure; the practical limits of grpc-gateway over HTTP. The FAQ entry shrinks to a one-line pointer at this page./howto/database,/howto/cache,/howto/messaging— the framework patterns for long-lived dependencies. ColdBrew is database / cache / broker agnostic, so these pages document the lifecycle hooks (PreStart/Stop) and worker integration (CBWorkerProvider) and ground each pattern in one concrete library: pgx for Postgres, go-redis for Redis/Valkey, IBM/sarama for Kafka, and nats.go for NATS.The Kafka example uses sarama's consumer-group API (
MarkMessageonly after handler success) so the at-least-once guarantee the surrounding prose claims is actually delivered. The NATS example usesChanQueueSubscribe+workers.ChannelWorker, which keeps the consumer loop in the framework and leaves the handler as a purefunc(ctx, info, *nats.Msg) error— broker-agnostic, unit-testable, and the channel is the seam.Configuration pattern
Every example loads user config through the cookiecutter's
config/package — extend theConfigstruct withenvconfigtags and access viaconfig.Get().FieldName, matching the cookiecutter's ownconfig/config.go. No rawos.Getenv.Cross-links
/howto/local-devnow points at the database, cache, and messaging how-tos next to the matching profile tables./howto/signalsstep 4 ("Stop workers") points at the messaging how-to for the consumer-drain pattern.Tests
/concepts/, key sections on/howto/streaming-rpcs/, andconfig.Get()-based examples on the three data how-tos.newHowtoPagesconstant intests/pages.tsis shared bycontent.spec.ts(SEO + TOC) andlinks.spec.ts(broken-link crawl) so adding a how-to only touches one list./concepts/added totopLevelPages, SEO, TOC, and broken-link crawl sets.Test plan
Summary by CodeRabbit
Documentation