docs: top-notch README (banner, story diagrams, screenshots) + ingest hardening#46
Merged
Merged
Conversation
…n ingest source-fetch retry - New README: generated banner, how-it-works + vs-RAG story SVGs, install/terminal and dashboard screenshots, Docker quick-start, BYOK, API, SDK, benchmark sections. - Widen getSourceWithRetry window (~16s) so large sources survive heavy concurrent ingestion; UI Ask falls back to the server key when no BYOK key is set.
Reviewer's GuideMajor README rewrite for launch (new visuals, clearer narrative, quick-start/Docker/BYOK/API/SDK/benchmark sections) plus ingest robustness under concurrent load and a more graceful BYOK UX in the local UI. Sequence diagram for BYOK Ask flow and fallback to server keysequenceDiagram
actor User
participant BrowserUI
participant EngineAPI
User->>BrowserUI: Click Ask
BrowserUI->>BrowserUI: ask()
BrowserUI->>EngineAPI: POST /v1/answer/treewalk
activate EngineAPI
alt BYOK header present (X-LLM-Api-Key)
EngineAPI-->>BrowserUI: 200 answer
BrowserUI->>BrowserUI: renderResult()
else No BYOK header
alt Server LLM key configured
EngineAPI-->>BrowserUI: 200 answer
BrowserUI->>BrowserUI: renderResult()
else No LLM credentials
EngineAPI-->>BrowserUI: 4xx { error: "no LLM credentials" }
deactivate EngineAPI
BrowserUI->>BrowserUI: ask() error handling
BrowserUI->>BrowserUI: openSettings()
BrowserUI->>BrowserUI: setStatus "Set your API key to ask questions."
end
end
Sequence diagram for getSourceWithRetry ingest backoff changessequenceDiagram
participant Pipeline
participant Storage
Pipeline->>Pipeline: getSourceWithRetry(ctx, storage, key)
loop Up to 16 attempts
Pipeline->>Storage: Get(ctx, key)
alt Get succeeds
Storage-->>Pipeline: io.ReadCloser, Metadata, nil
Pipeline-->>Pipeline: return source and break
else err is storage.ErrNotFound
Storage-->>Pipeline: _, _, ErrNotFound
Pipeline->>Pipeline: incremental backoff (125ms * (i+1))
else other error
Storage-->>Pipeline: _, _, err
Pipeline-->>Pipeline: return err and break
end
end
alt All attempts exhausted
Pipeline-->>Pipeline: return lastErr
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThree independent changes: ChangesRuntime behavior changes
README overhaul
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The UI
ask()handler now infers missing-credentials errors via string matching (/no LLM credentials|X-LLM-Api-Key/i); consider returning a structured error code or type from the backend so the client can branch on a stable signal instead of brittle message text. - The
getSourceWithRetrybackoff window has been significantly widened with hardcoded constants; you might want to make the max attempts and base delay configurable (e.g., via config or env) so operators can tune ingest latency vs. resilience for their environment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The UI `ask()` handler now infers missing-credentials errors via string matching (`/no LLM credentials|X-LLM-Api-Key/i`); consider returning a structured error code or type from the backend so the client can branch on a stable signal instead of brittle message text.
- The `getSourceWithRetry` backoff window has been significantly widened with hardcoded constants; you might want to make the max attempts and base delay configurable (e.g., via config or env) so operators can tune ingest latency vs. resilience for their environment.
## Individual Comments
### Comment 1
<location path="localapp/index.html" line_range="386-388" />
<code_context>
- if(!r.ok){ out.innerHTML=`<div class="res card"><div class="body err">Error: ${esc(d.error||JSON.stringify(d))}</div></div>`; return; }
+ if(!r.ok){
+ const msg=d.error||JSON.stringify(d);
+ if(/no LLM credentials|X-LLM-Api-Key/i.test(msg)){
+ out.innerHTML=""; openSettings();
+ document.getElementById("setStatus").innerHTML='<span style="color:var(--warn)">Set your API key to ask questions.</span>';
+ return;
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Relying on server error strings for credential detection is brittle and may cause extra round-trips.
The previous client-side `apiKey` check avoided a request and didn’t depend on specific error text. This new flow relies on matching `no LLM credentials|X-LLM-Api-Key` in the server message, which is fragile if backend wording changes and always sends a request even when we already know there’s no key. Consider restoring a lightweight client-side guard when the UI knows no key is set, or have the backend return a structured/typed error that you can check instead of regexing the message text.
</issue_to_address>
### Comment 2
<location path="localapp/index.html" line_range="387" />
<code_context>
+ if(!r.ok){
+ const msg=d.error||JSON.stringify(d);
+ if(/no LLM credentials|X-LLM-Api-Key/i.test(msg)){
+ out.innerHTML=""; openSettings();
+ document.getElementById("setStatus").innerHTML='<span style="color:var(--warn)">Set your API key to ask questions.</span>';
+ return;
</code_context>
<issue_to_address>
**suggestion:** Clearing the result area on missing-credentials errors may degrade UX by removing prior answers.
This now clears `out.innerHTML` on missing-API-key, removing any prior answer the user may still be reading. Previously we never entered the ask flow, so the last result stayed visible. Consider keeping the existing result card and only updating the settings/status UI, so going to settings doesn’t wipe the current content.
</issue_to_address>
### Comment 3
<location path="README.md" line_range="139" />
<code_context>
- }'
-# → {"sections": [{"id":"sec_...","title":"...","content":"..."}]}
-```
+Every run writes a manifest stamped with model, hardware, library versions, and git commit, so numbers are reproducible and audit-able. Headline results are published with the launch.
-## HTTP API (v1)
</code_context>
<issue_to_address>
**issue (typo):** Use the standard spelling "auditable" instead of "audit-able".
The hyphenated form is nonstandard; please update it for consistency with typical technical usage.
```suggestion
Every run writes a manifest stamped with model, hardware, library versions, and git commit, so numbers are reproducible and auditable. Headline results are published with the launch.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+386
to
+388
| if(/no LLM credentials|X-LLM-Api-Key/i.test(msg)){ | ||
| out.innerHTML=""; openSettings(); | ||
| document.getElementById("setStatus").innerHTML='<span style="color:var(--warn)">Set your API key to ask questions.</span>'; |
There was a problem hiding this comment.
suggestion (bug_risk): Relying on server error strings for credential detection is brittle and may cause extra round-trips.
The previous client-side apiKey check avoided a request and didn’t depend on specific error text. This new flow relies on matching no LLM credentials|X-LLM-Api-Key in the server message, which is fragile if backend wording changes and always sends a request even when we already know there’s no key. Consider restoring a lightweight client-side guard when the UI knows no key is set, or have the backend return a structured/typed error that you can check instead of regexing the message text.
| if(!r.ok){ | ||
| const msg=d.error||JSON.stringify(d); | ||
| if(/no LLM credentials|X-LLM-Api-Key/i.test(msg)){ | ||
| out.innerHTML=""; openSettings(); |
There was a problem hiding this comment.
suggestion: Clearing the result area on missing-credentials errors may degrade UX by removing prior answers.
This now clears out.innerHTML on missing-API-key, removing any prior answer the user may still be reading. Previously we never entered the ask flow, so the last result stayed visible. Consider keeping the existing result card and only updating the settings/status UI, so going to settings doesn’t wipe the current content.
| }' | ||
| # → {"sections": [{"id":"sec_...","title":"...","content":"..."}]} | ||
| ``` | ||
| Every run writes a manifest stamped with model, hardware, library versions, and git commit, so numbers are reproducible and audit-able. Headline results are published with the launch. |
There was a problem hiding this comment.
issue (typo): Use the standard spelling "auditable" instead of "audit-able".
The hyphenated form is nonstandard; please update it for consistency with typical technical usage.
Suggested change
| Every run writes a manifest stamped with model, hardware, library versions, and git commit, so numbers are reproducible and audit-able. Headline results are published with the launch. | |
| Every run writes a manifest stamped with model, hardware, library versions, and git commit, so numbers are reproducible and auditable. Headline results are published with the launch. |
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.
Rewrites the README for launch and hardens ingest under concurrency.
how-it-works+vs-ragstory SVGs, install/terminal + dashboard screenshots, Docker quick-start, BYOK, API table, SDK + benchmark (methodology) sections.getSourceWithRetry(~16s) so large sources survive heavy concurrent ingestion; UI Ask gracefully falls back to the server key when no BYOK key is set.Build + vet clean; ingest + api tests pass.
Summary by Sourcery
Revise the README for launch with clearer positioning, quick-start Docker instructions, BYOK, API/SDK/benchmark sections, and updated visuals while hardening ingest under concurrent load and making the UI Ask flow gracefully handle missing API keys.
Enhancements:
Documentation:
Summary by CodeRabbit
Documentation
Improvements