Feat/spell check batch#650
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR replaces the embedded fuzzy-model spellcheck engine with an HTTP-backed LanguageTool service. Configuration is added to wire the LanguageTool URL, the core service is rewritten as an HTTP client, data types gain a Suggestions field and batch support, all tests are refactored for mock HTTP servers, a batch endpoint is added, and Docker Compose infrastructure is configured. ChangesSpellcheck Engine Migration to LanguageTool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/apis/text/spell-check.md (1)
68-70:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRemove incorrect ASCII-only documentation; the service supports full Unicode input.
Lines 68-70 claim only ASCII letter sequences are spell-checked, but this contradicts both
spell-check.ymlline 17 and the actual implementation. The code converts input to runes and works with Unicode offsets; the test suite explicitly validates this with non-ASCII characters like "é". Update lines 68-70 to correctly describe that the service supports Unicode input including accented and non-Latin characters.🤖 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/text/spell-check.md` around lines 68 - 70, Replace the incorrect assertion that only ASCII letter sequences are spell-checked with a statement that the spell-check service supports full Unicode input (including accented and non-Latin characters); update the copy around the previous lines 68–70 in spell-check.md to mention that input is converted to runes/unicode codepoints and Unicode offsets are used for position counting (consistent with spell-check.yml and the implementation and tests that validate characters like "é").
🤖 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/text/spellcheck/service_test.go`:
- Around line 17-20: The mock LanguageTool server in the httptest.NewServer
handler currently accepts any request; update the handler inside the
httptest.NewServer(http.HandlerFunc(...)) to assert r.Method == "POST" and
r.URL.Path == "/v2/check" before returning the ltResponse{Matches: matches}; if
the method or path mismatch, return a non-200 (e.g. 405 for wrong method, 404
for wrong path) so tests fail when code calls the wrong endpoint. Locate the
handler closure in service_test.go where ltResponse and matches are used and add
these conditional checks and appropriate http.Error responses.
- Around line 83-84: The test currently calls
NewService("http://localhost:19999") which hardcodes a port and can be flaky;
replace that with a closed httptest server URL: create an
httptest.NewServer(nil) (or a simple handler), capture ts.URL, call ts.Close()
to guarantee the backend is unreachable, then call
NewService(ts.URL).Check("hello") and assert an error; update the test in
service_test.go to reference NewService and Check accordingly so no fixed
localhost port is used.
In `@apps/api/services/text/spellcheck/service.go`:
- Around line 64-67: Check HTTP status on the LanguageTool response before
attempting to decode: in the block that reads resp into ltResp, validate
resp.StatusCode is 2xx (e.g., >=200 && <300) and return an error that includes
the non-2xx status (and optionally response body) instead of proceeding to
json.Decode; reference the existing resp and ltResponse variables. Also harden
buildResult by validating that each match's Offset and Length are non-negative
and that Offset+Length does not exceed len(runes) before slicing or appending
(check Offset >= 0, Length >= 0, and Offset+Length <= len(runes)), and return a
clear error or skip invalid matches to avoid panics when constructing the
Result.
- Around line 93-95: The loop over matches uses external m.Offset and m.Length
directly for rune slicing and only checks the upper bound; add lower-bound
validation so you skip or clamp any spans where m.Offset < 0 or m.Length < 0
(and keep the existing m.Offset+m.Length <= len(runes)) before any
rune/corrected slicing. Update the validation around the matches iteration (the
block referencing matches, m.Offset, m.Length, and runes) and the later
corrected-slicing code that builds the corrected string (the code that slices
corrected using m.Offset/m.Length and applies m.Replacements) to ensure both
offset and length are non-negative and that m.Offset+m.Length does not exceed
the target slice length.
In `@apps/api/services/text/spellcheck/transport_http_test.go`:
- Around line 234-243: The test uses a hard-coded external address in
setupRouter("http://localhost:19999") which can intermittently hit a real
process; make the failure deterministic by creating an httptest server and using
its URL after closing it (e.g., create an httptest.NewServer(...), call Close()
and pass ts.URL into setupRouter) so the service URL always points to a closed
endpoint; update the test in transport_http_test.go to replace the literal
"http://localhost:19999" with the closed httptest server URL when calling
setupRouter.
In `@docs/apis/text/spell-check.md`:
- Line 136: Replace the incorrect sentence "This endpoint always returns `200`
with a result for every input text. There are no per-item errors —
spell-checking is pure local computation with no external dependencies." with a
corrected description that states the spell-check endpoint now calls an external
LanguageTool HTTP service (running in a separate container) and therefore can
fail if that service is unavailable; keep the note that an unavailable
LanguageTool results in a 500 for the entire request and clarify whether
per-item errors are returned or the request is aborted (choose and document the
actual behavior implemented by the migration to LanguageTool).
---
Outside diff comments:
In `@docs/apis/text/spell-check.md`:
- Around line 68-70: Replace the incorrect assertion that only ASCII letter
sequences are spell-checked with a statement that the spell-check service
supports full Unicode input (including accented and non-Latin characters);
update the copy around the previous lines 68–70 in spell-check.md to mention
that input is converted to runes/unicode codepoints and Unicode offsets are used
for position counting (consistent with spell-check.yml and the implementation
and tests that validate characters like "é").
🪄 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: 0965d225-0a8e-42c2-bae9-9a6764bb485f
📒 Files selected for processing (15)
apps/api/app/routes_v1.goapps/api/go.modapps/api/platform/config/config.goapps/api/services/text/router.goapps/api/services/text/spellcheck/service.goapps/api/services/text/spellcheck/service_test.goapps/api/services/text/spellcheck/transport_http.goapps/api/services/text/spellcheck/transport_http_test.goapps/api/services/text/spellcheck/type.goapps/dashboard/config/api_catalog.ymlapps/dashboard/config/api_docs/spell-check.ymldocs/apis/text/spell-check.mddocs/design-plans/2026-05-19-spellcheck-engine-languagetool.mdinfra/docker/.env.exampleinfra/docker/docker-compose.dev.yml
💤 Files with no reviewable changes (1)
- apps/api/go.mod
d124963 to
7c73d87
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
… contract, replace fixed ports in tests, fix docs
There was a problem hiding this comment.
Good work ! The migration from sajari/fuzzy to LanguageTool is the right call, and the design document does a solid job laying out the alternatives and trade-offs, including the future ByT5 path, which is useful context to have written down. The type separation into type.go is clean, and rewriting the tests to mock the LT HTTP contract with httptest.Server is the correct approach: they're now testing the actual integration boundary rather than a stub.
A few things to address before merging:
CheckBatchis sequential. It callsCheckin a loop, meaning 50 texts = 50 serial HTTP round-trips to LanguageTool. The batch method should fan out concurrently using a semaphore +sync.WaitGroupso callers get an actual throughput benefit. Use in-band errors (failed item returns a zero-value result, batch continues) — do not useerrgroup, which would abort the entire batch on the first LT error:
func (s *Service) CheckBatch(texts []string) []Result {
const maxWorkers = 10
results := make([]Result, len(texts))
sem := make(chan struct{}, maxWorkers)
var wg sync.WaitGroup
for i, text := range texts {
wg.Add(1)
sem <- struct{}{}
go func(i int, text string) {
defer wg.Done()
defer func() { <-sem }()
r, err := s.Check(text)
if err != nil {
results[i] = Result{} // zero-value, batch continues
return
}
results[i] = r
}(i, text)
}
wg.Wait()
return results
}The semaphore caps concurrency at 10 so a large batch can't flood the LanguageTool instance. Results are written to pre-allocated index slots — input order is always preserved without sorting. See networking/whois/service.go:LookupBatch for the canonical in-codebase reference.
-
LanguageTool starts unconditionally in dev. The
languagetoolservice indocker-compose.dev.ymladds ~2 GB RAM overhead for every developer regardless of whether they're touching spell-check. Wrapping it in a Compose profile (e.g.profiles: [languagetool]) would make it opt-in:COMPOSE_PROFILES=languagetool docker compose upfor those who need it. -
condition: service_startedraces. LanguageTool takes ~10 seconds to be ready after the process starts (the design doc calls this out). Ahealthcheckblock on the service pluscondition: service_healthyon theapidependency would prevent startup failures. Note: theerikvl87/languagetoolimage is Alpine-based and does not includecurl; usewgetinstead:healthcheck: test: ["CMD-SHELL", "wget -qO- http://localhost:8010/v2/languages > /dev/null 2>&1"] interval: 5s timeout: 5s retries: 10 start_period: 30s
-
No production compose change. The PR only touches
docker-compose.dev.yml; we need to con figure language tools in the docker-compose.yml so it works in prod too.
None of these are hard blockers and the core of the work is solid. Great contribution.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/text/spellcheck/service.go`:
- Around line 116-117: The span validation in buildResult is vulnerable to
integer overflow when computing m.Offset+m.Length before slicing; replace checks
like "m.Offset+m.Length > len(runes)" with overflow-safe guards: ensure m.Offset
>= 0, m.Offset <= len(runes), m.Length >= 0, and m.Length <= len(runes)-m.Offset
(and similarly use len(corrected) when slicing corrected). Update both
validation sites in buildResult (the checks around runes and corrected slices)
to use these safe comparisons, and also wrap goroutine calls in CheckBatch that
invoke s.Check with a defer-recover to avoid process-wide panics if a slice
still panics. Ensure you reference the symbols m.Offset, m.Length, runes,
corrected, buildResult, CheckBatch and s.Check when making the changes.
In `@docs/apis/text/spell-check.md`:
- Around line 136-148: Update the ambiguous statement "This endpoint always
returns `200`" in the spell-check batch docs so it explicitly distinguishes
validation/early-failure 500 responses from per-item failures: change the
sentence to say the endpoint returns `200` only after successful
validation/processing start (and will return `500` for validation or
pre-processing failures such as LanguageTool being completely unavailable), and
clarify that once processing has started, individual item failures yield
zero-value results (`corrected: ""`, `corrections: null`) while the overall
response is still `200`; ensure the "Batch Error Codes" table remains and add a
short sentence linking each code to these conditions (validation/missing body ->
`400`/`422`, unexpected/pre-processing failure -> `500`, per-item LanguageTool
failures -> zero-value items with `200`).
🪄 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: 8a3f6c9a-7e9d-4349-ab67-70f4dd531c61
📒 Files selected for processing (16)
apps/api/app/routes_v1.goapps/api/go.modapps/api/platform/config/config.goapps/api/services/text/router.goapps/api/services/text/spellcheck/service.goapps/api/services/text/spellcheck/service_test.goapps/api/services/text/spellcheck/transport_http.goapps/api/services/text/spellcheck/transport_http_test.goapps/api/services/text/spellcheck/type.goapps/dashboard/config/api_catalog.ymlapps/dashboard/config/api_docs/spell-check.ymldocs/apis/text/spell-check.mddocs/design-plans/2026-05-19-spellcheck-engine-languagetool.mdinfra/docker/.env.exampleinfra/docker/docker-compose.dev.ymlinfra/docker/docker-compose.yml
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/config/api_docs/spell-check.yml
2 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores
Tests