feat: add OCR input logging and detail view#2860
Merged
Conversation
18 tasks
Collaborator
Author
This was referenced Apr 20, 2026
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds end-to-end OCR support: DB migration for Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Plugin as LoggerPlugin
participant Store as LogStore
participant DB as Database
participant UI as UI
Client->>Plugin: Send OCR Request (document/image)
Plugin->>Plugin: PreLLMHook captures OCR document\ninto InitialLogData.OCRInput
Plugin->>Store: insertInitialLogEntry(OCRInputParsed = InitialData.OCRInput)
Store->>Store: SerializeFields(OCRInputParsed → OCRInput JSON)
Store->>DB: INSERT log (ocr_input column)
DB-->>Store: Confirm insert
Plugin->>Store: buildCompleteLogEntryFromPending\n(populate OCRInputParsed / OCROutputParsed)
Store->>Store: SerializeFields(final OCR fields → JSON)
Store->>DB: UPDATE log with final OCR data
UI->>DB: Fetch log entry (contains ocr_input/ocr_output)
DB-->>UI: Return log row
UI->>UI: DeserializeFields(OCRInput JSON → parsed)
UI->>UI: Render OCRView (pages, text, images, metadata)
UI-->>Client: Display OCR results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Confidence Score: 5/5Safe to merge — the only finding is a minor omission in a coarse size-estimation helper that the codebase itself documents as approximate. All remaining findings are P2. The migration, Go struct fields, serialization hooks, payload utilities, and UI types are all symmetric with the existing modality pattern. No logic errors or correctness issues were found. plugins/logging/writer.go — Important Files Changed
Reviews (3): Last reviewed commit: "fix: ocr logging fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 3
🧹 Nitpick comments (1)
ui/app/workspace/logs/views/ocrView.tsx (1)
20-20: Rename file to PascalCase for component naming consistency.
OCRViewexport is PascalCase, but filenameocrView.tsxis not. Rename toOCRView.tsx(and update imports) to match the repository TSX convention.As per coding guidelines: "React component files must use PascalCase for component exports and filenames."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/logs/views/ocrView.tsx` at line 20, Rename the file to match the exported React component by changing the filename from ocrView.tsx to OCRView.tsx and update any imports referencing the old path; locate the component function OCRView and ensure all import statements across the codebase that import "./ocrView" (or similar) are updated to "./OCRView" so the PascalCase filename matches the PascalCase export and adheres to the project convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/logging/main.go`:
- Around line 492-494: initialData currently stores the raw OCR document
(initialData.OCRInput = &req.OCRRequest.Document), which can persist signed
URLs/credentials; before assigning, create a sanitized copy of
req.OCRRequest.Document that redacts or strips query strings from known URL
fields (e.g., document_url, image_url) or replaces them with a safe placeholder,
and assign that sanitized copy to initialData.OCRInput; update any helper used
to deep-copy or marshal the Document to ensure nested URL fields are handled and
preserve other fields in req.OCRRequest.Params as before.
In `@ui/app/workspace/logs/views/ocrView.tsx`:
- Around line 149-156: The pagination buttons in ocrView.tsx lack test
selectors—update the two Button JSX elements that call goToPrevious and goToNext
(the buttons surrounding ChevronLeft/ChevronRight and showing Page {currentIndex
+ 1} / {totalPages}) to include stable data-testid attributes (e.g.,
data-testid="ocr-prev-page-button" and data-testid="ocr-next-page-button") so
E2E tests can reliably target them; keep the existing props (variant, size,
onClick, aria-label, title) and only add the data-testid attributes.
---
Nitpick comments:
In `@ui/app/workspace/logs/views/ocrView.tsx`:
- Line 20: Rename the file to match the exported React component by changing the
filename from ocrView.tsx to OCRView.tsx and update any imports referencing the
old path; locate the component function OCRView and ensure all import statements
across the codebase that import "./ocrView" (or similar) are updated to
"./OCRView" so the PascalCase filename matches the PascalCase export and adheres
to the project convention.
🪄 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
Run ID: 9ad991a5-0243-4f0e-b901-1b5155e3f0a2
📒 Files selected for processing (10)
framework/logstore/migrations.goframework/logstore/payload.goframework/logstore/tables.goplugins/logging/main.goplugins/logging/operations.goplugins/logging/utils.goplugins/logging/writer.goui/app/workspace/logs/sheets/logDetailView.tsxui/app/workspace/logs/views/ocrView.tsxui/lib/types/logs.ts
💤 Files with no reviewable changes (1)
- plugins/logging/utils.go
|
|
Pratham-Mishra04
force-pushed
the
04-20-fix_ocr_logging_fixes
branch
from
7f7228a to
75d28ed
Compare
Pratham-Mishra04
force-pushed
the
04-20-fix_minor_mcp_and_ui_fixes
branch
from
df50781 to
45969f8
Compare
Pratham-Mishra04
force-pushed
the
04-20-fix_ocr_logging_fixes
branch
from
7f7228a to
75d28ed
Compare
18 tasks
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plugins/logging/main.go (1)
491-493: Prefer copying OCR document and usingbifrost.Ptr()here.On Line [493], taking
&req.OCRRequest.Documentaliases request-owned memory. A copied value keeps log input isolated and aligns with repo pointer conventions.♻️ Proposed refactor
case schemas.OCRRequest: initialData.Params = req.OCRRequest.Params - initialData.OCRInput = &req.OCRRequest.Document + doc := req.OCRRequest.Document + initialData.OCRInput = bifrost.Ptr(doc)Based on learnings: In the maximhq/bifrost repository, prefer using
bifrost.Ptr()to create pointers instead of the address operator (&) for consistency across code paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/main.go` around lines 491 - 493, The code currently takes the address of request-owned memory (&req.OCRRequest.Document) when handling schemas.OCRRequest, which aliases request memory; instead, make a copy of req.OCRRequest.Document and assign a pointer to that copy using bifrost.Ptr() (e.g., create a local copy var docCopy = req.OCRRequest.Document and set initialData.OCRInput = bifrost.Ptr(docCopy)) so logs own their input; update the case handling in the same switch (schemas.OCRRequest) to use the copied value and bifrost.Ptr() for consistency with the repo conventions.ui/lib/types/logs.ts (1)
229-233: TightenOCRDocumentto a discriminated union to prevent invalid states.Current typing allows
type: "document_url"withoutdocument_url(or with both URLs). Make URL presence/type coupling strict.Proposed type-safe shape
-export interface OCRDocument { - type: "document_url" | "image_url"; - document_url?: string; - image_url?: string; -} +export type OCRDocument = + | { + type: "document_url"; + document_url: string; + image_url?: never; + } + | { + type: "image_url"; + image_url: string; + document_url?: never; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/logs.ts` around lines 229 - 233, The OCRDocument interface allows invalid states because type is not tied to which URL is present; replace the current OCRDocument declaration with a discriminated union so that when type === "document_url" the shape requires document_url: string and forbids image_url, and when type === "image_url" it requires image_url: string and forbids document_url; update any callers of OCRDocument to handle the two union branches (narrow by the type field) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/logging/main.go`:
- Around line 491-493: The code currently takes the address of request-owned
memory (&req.OCRRequest.Document) when handling schemas.OCRRequest, which
aliases request memory; instead, make a copy of req.OCRRequest.Document and
assign a pointer to that copy using bifrost.Ptr() (e.g., create a local copy var
docCopy = req.OCRRequest.Document and set initialData.OCRInput =
bifrost.Ptr(docCopy)) so logs own their input; update the case handling in the
same switch (schemas.OCRRequest) to use the copied value and bifrost.Ptr() for
consistency with the repo conventions.
In `@ui/lib/types/logs.ts`:
- Around line 229-233: The OCRDocument interface allows invalid states because
type is not tied to which URL is present; replace the current OCRDocument
declaration with a discriminated union so that when type === "document_url" the
shape requires document_url: string and forbids image_url, and when type ===
"image_url" it requires image_url: string and forbids document_url; update any
callers of OCRDocument to handle the two union branches (narrow by the type
field) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 396fe7cf-2fad-4ed0-a5ac-e84ab10a0818
📒 Files selected for processing (10)
framework/logstore/migrations.goframework/logstore/payload.goframework/logstore/tables.goplugins/logging/main.goplugins/logging/operations.goplugins/logging/utils.goplugins/logging/writer.goui/app/workspace/logs/sheets/logDetailView.tsxui/app/workspace/logs/views/ocrView.tsxui/lib/types/logs.ts
💤 Files with no reviewable changes (1)
- plugins/logging/utils.go
✅ Files skipped from review due to trivial changes (2)
- ui/app/workspace/logs/sheets/logDetailView.tsx
- framework/logstore/migrations.go
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/logging/operations.go
- framework/logstore/tables.go
- framework/logstore/payload.go
- plugins/logging/writer.go
- ui/app/workspace/logs/views/ocrView.tsx
Pratham-Mishra04
force-pushed
the
04-20-fix_ocr_logging_fixes
branch
from
75d28ed to
bbec714
Compare
Pratham-Mishra04
force-pushed
the
04-20-fix_minor_mcp_and_ui_fixes
branch
from
45969f8 to
525f6ec
Compare
Merged
18 tasks
Contributor
Merge activity
|
akshaydeo
changed the base branch from
04-20-fix_minor_mcp_and_ui_fixes
to
graphite-base/2860
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
dominictayloruk
pushed a commit
to dominictayloruk/bifrost
that referenced
this pull request
Apr 21, 2026
## Summary Adds full logging support for OCR requests, storing the OCR input document and output response in the log store and rendering them in the log detail view in the UI. ## Changes - Added `ocr_input` column to the `logs` table via a new database migration (`migrationAddOCRInputColumn`). - Added `OCRInput`/`OCROutput` fields and their parsed counterparts to the `Log` struct, with serialization and deserialization logic. - Included `ocr_input` and `ocr_output` in payload extraction, clearing, merging, and field-clearing logic. - Populated `OCRInput` from the incoming `OCRRequest.Document` in the `PreLLMHook` and propagated it through `insertInitialLogEntry` and `buildCompleteLogEntryFromPending`. - Removed the previous workaround in `extractInputHistory` that represented the OCR document as a synthetic chat message with a stripped URL, since the input is now stored directly as a structured field. - Added TypeScript types for `OCRDocument`, `OCRPage`, `OCRPageImage`, `OCRPageDimensions`, `OCRUsageInfo`, and `BifrostOCRResponse`, and exposed `ocr_input`/`ocr_output` on `LogEntry`. - Added a new `OCRView` React component that renders the OCR input (type and URL), output metadata (pages processed, document size), per-page markdown content with a paginator, and any extracted images. - Integrated `OCRView` into the log detail view's messages tab. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test 1. Send an OCR request through Bifrost with a valid document or image URL. 2. Open the Logs view in the UI and select the resulting log entry. 3. In the **Messages** tab, verify the OCR Input section shows the document type and URL. 4. Verify the OCR Output section shows usage info, per-page markdown, and any extracted images with pagination if multiple pages are present. 5. Confirm that restarting the service after the migration runs does not re-apply the migration. ```sh go test ./framework/logstore/... ./plugins/logging/... cd ui pnpm i pnpm build ``` ## Breaking changes - [ ] Yes - [x] No ## Security considerations The previous implementation stripped query parameters from OCR document URLs before logging to avoid persisting sensitive tokens (e.g., pre-signed URLs). The new `OCRInput` field stores the raw `OCRDocument` struct, which may include full URLs with query parameters. Ensure that pre-signed or otherwise sensitive URLs are not passed directly as document or image URLs if log retention is a concern. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
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
3 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.
Summary
Adds full logging support for OCR requests, storing the OCR input document and output response in the log store and rendering them in the log detail view in the UI.
Changes
ocr_inputcolumn to thelogstable via a new database migration (migrationAddOCRInputColumn).OCRInput/OCROutputfields and their parsed counterparts to theLogstruct, with serialization and deserialization logic.ocr_inputandocr_outputin payload extraction, clearing, merging, and field-clearing logic.OCRInputfrom the incomingOCRRequest.Documentin thePreLLMHookand propagated it throughinsertInitialLogEntryandbuildCompleteLogEntryFromPending.extractInputHistorythat represented the OCR document as a synthetic chat message with a stripped URL, since the input is now stored directly as a structured field.OCRDocument,OCRPage,OCRPageImage,OCRPageDimensions,OCRUsageInfo, andBifrostOCRResponse, and exposedocr_input/ocr_outputonLogEntry.OCRViewReact component that renders the OCR input (type and URL), output metadata (pages processed, document size), per-page markdown content with a paginator, and any extracted images.OCRViewinto the log detail view's messages tab.Type of change
Affected areas
How to test
Breaking changes
Security considerations
The previous implementation stripped query parameters from OCR document URLs before logging to avoid persisting sensitive tokens (e.g., pre-signed URLs). The new
OCRInputfield stores the rawOCRDocumentstruct, which may include full URLs with query parameters. Ensure that pre-signed or otherwise sensitive URLs are not passed directly as document or image URLs if log retention is a concern.Checklist
docs/contributing/README.mdand followed the guidelines