Feat/lightspeed notebook chat#2754
Open
its-mitesh-kumar wants to merge 7 commits intoredhat-developer:mainfrom
Open
Feat/lightspeed notebook chat#2754its-mitesh-kumar wants to merge 7 commits intoredhat-developer:mainfrom
its-mitesh-kumar wants to merge 7 commits intoredhat-developer:mainfrom
Conversation
Signed-off-by: its-mitesh-kumar <itsmiteshkumar98@gmail.com>
Signed-off-by: its-mitesh-kumar <itsmiteshkumar98@gmail.com>
Signed-off-by: its-mitesh-kumar <itsmiteshkumar98@gmail.com>
Signed-off-by: its-mitesh-kumar <itsmiteshkumar98@gmail.com>
Signed-off-by: its-mitesh-kumar <itsmiteshkumar98@gmail.com>
Signed-off-by: its-mitesh-kumar <itsmiteshkumar98@gmail.com>
Signed-off-by: its-mitesh-kumar <itsmiteshkumar98@gmail.com>
its-mitesh-kumar
requested review from
a team,
karthikjeeyar,
rohitkrai03 and
yangcao77
as code owners
Contributor
|
This pull request adds a new top-level directory under |
Review Summary by QodoAdd notebook chat feature with document management and UI components
WalkthroughsDescription• Implemented comprehensive notebook chat feature with document management capabilities • Added NotebooksApiClient with new methods for session and document operations: createSession, uploadDocument, listDocuments, deleteDocument, getDocumentStatus, and querySession • Extended NotebooksAPI interface with document operations and session management • Created notebook-specific types for documents, sessions, and responses • Implemented file upload validation utilities with support for file type, size, and count constraints • Added React Query hooks for notebook operations: useCreateNotebook, useUploadDocument, useDocumentStatusPolling, useNotebookDocuments, and useCreateNotebookMessage • Built complete UI components for notebook functionality: NotebookView, AddDocumentModal, DocumentSidebar, OverwriteConfirmModal, FileTypeIcon, UploadResourceScreen • Integrated notebook view into main LightSpeedChat component with notebook creation and selection • Added comprehensive test coverage for utilities, hooks, and components • Provided translations in 6 languages (English, Japanese, German, French, Spanish, Italian) for all notebook UI elements • Enhanced useConversationMessages hook with createMessageOverride parameter for custom message creation logic • Configured backend notebook query defaults and metadata handling Diagramflowchart LR
A["NotebooksApiClient"] -- "session & document methods" --> B["NotebooksAPI Interface"]
B -- "uses" --> C["Notebook Types"]
D["File Upload Utils"] -- "validates" --> E["AddDocumentModal"]
E -- "uploads via" --> A
F["useCreateNotebook"] -- "creates" --> A
G["useUploadDocument"] -- "uploads" --> A
H["useDocumentStatusPolling"] -- "tracks" --> A
I["useNotebookDocuments"] -- "lists" --> A
J["useCreateNotebookMessage"] -- "queries" --> A
E --> K["NotebookView"]
L["DocumentSidebar"] -- "displays" --> K
M["OverwriteConfirmModal"] -- "confirms" --> K
K -- "integrated into" --> N["LightSpeedChat"]
O["Translations"] -- "supports" --> K
File Changes1. workspaces/lightspeed/plugins/lightspeed/src/api/NotebooksApiClient.ts
|
Code Review by Qodo
|
|
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
Comment on lines
+223
to
+236
| if (!response.ok) { | ||
| const reader = response.body.getReader(); | ||
| const { done, value } = await reader.read(); | ||
| const text = done ? '' : new TextDecoder('utf-8').decode(value); | ||
| const errorMessage = JSON.parse(text); | ||
| if (errorMessage?.error) { | ||
| throw new Error( | ||
| `failed to query notebook session: ${errorMessage.error}`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return response.body.getReader(); | ||
| } |
There was a problem hiding this comment.
1. Stream reader locked twice 🐞 Bug ≡ Correctness
NotebooksApiClient.querySession() calls response.body.getReader() in the error path and then calls getReader() again to return a reader, which will throw because the stream is already locked and also may let non-OK responses proceed without throwing.
Agent Prompt
### Issue description
`NotebooksApiClient.querySession()` locks the response stream by calling `response.body.getReader()` in the error branch and then calls `getReader()` again, which will throw at runtime. It also doesn't reliably throw for non-OK responses.
### Issue Context
This method is used for notebook chat streaming; failed requests should surface a clean error without breaking the stream API.
### Fix Focus Areas
- workspaces/lightspeed/plugins/lightspeed/src/api/NotebooksApiClient.ts[205-236]
### Suggested fix
- If `!response.ok`, read the error via `await response.text()` (or reuse a single reader and `releaseLock()`), then `throw` unconditionally.
- Only call `response.body.getReader()` once (in the success path) and return that reader.
- Wrap JSON parsing of error text in `try/catch` and fall back to raw text when not JSON.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+411
to
+414
| req.body.model = notebookModel; | ||
| req.body.provider = notebookProvider; | ||
| req.body.vector_store_ids = [sessionId]; | ||
| req.body.media_type = 'application/json'; |
There was a problem hiding this comment.
2. Empty model/provider forced 🐞 Bug ≡ Correctness
The notebooks backend query route always overwrites req.body.model/provider from config and defaults missing config to empty strings, which violates the system's own non-empty model/provider requirement and can cause all notebook queries to fail when queryDefaults are not configured.
Agent Prompt
### Issue description
Notebook query requests are forced to include `model` and `provider` values from config, but the code defaults them to empty strings and always overwrites the request body. This creates invalid requests when config is missing.
### Issue Context
The backend already treats empty `model`/`provider` as invalid (`validateCompletionsRequest`). Notebook queries should either:
- fail fast with a clear configuration error, or
- avoid setting these fields unless valid values are present.
### Fix Focus Areas
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts[75-82]
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts[411-414]
### Suggested fix
- Change defaults to `undefined` (or use `config.getString(...)` and throw on startup if missing).
- In the handler, only assign `req.body.model/provider` when the configured value is a non-empty string; otherwise return `400/500` with a clear message indicating missing `lightspeed.aiNotebooks.queryDefaults.*`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+40
to
+62
| export const NOTEBOOK_ALLOWED_EXTENSIONS: Record<string, string[]> = { | ||
| 'text/plain': ['.txt', '.log'], | ||
| 'text/markdown': ['.md'], | ||
| 'application/pdf': ['.pdf'], | ||
| 'application/json': ['.json'], | ||
| 'application/x-yaml': ['.yaml', '.yml'], | ||
| 'application/vnd.openxmlformats-officedocument.wordprocessingml.document': [ | ||
| '.docx', | ||
| ], | ||
| 'application/vnd.oasis.opendocument.text': ['.odt'], | ||
| }; | ||
|
|
||
| export const NOTEBOOK_EXTENSION_TO_FILE_TYPE: Record<string, string> = { | ||
| '.txt': 'txt', | ||
| '.md': 'md', | ||
| '.pdf': 'pdf', | ||
| '.json': 'json', | ||
| '.yaml': 'yaml', | ||
| '.yml': 'yaml', | ||
| '.log': 'log', | ||
| '.docx': 'txt', | ||
| '.odt': 'txt', | ||
| }; |
There was a problem hiding this comment.
3. Docx/odt upload corrupted 🐞 Bug ≡ Correctness
The frontend accepts .docx/.odt and maps them to fileType 'txt', but the backend only supports parsing txt/md/log as UTF-8 text, so these binary formats will be ingested as garbage content.
Agent Prompt
### Issue description
The UI currently allows `.docx`/`.odt` uploads and maps them to `fileType='txt'`, but the backend parses `txt` as UTF-8 text, corrupting binary document content.
### Issue Context
Backend supported types are `md/txt/pdf/json/yaml/yml/log/url` and text parsing is `buffer.toString('utf-8')`.
### Fix Focus Areas
- workspaces/lightspeed/plugins/lightspeed/src/const.ts[40-62]
- workspaces/lightspeed/plugins/lightspeed/src/hooks/notebooks/useUploadDocument.ts[34-52]
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/constant.ts[36-48]
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/documents/fileParser.ts[41-59]
### Suggested fix
Choose one:
1) **Disallow docx/odt**: remove `.docx`/`.odt` from `NOTEBOOK_ALLOWED_EXTENSIONS` and `NOTEBOOK_EXTENSION_TO_FILE_TYPE`, and update the upload modal `infoText` translations accordingly.
2) **Add real backend support**: extend `SupportedFileType` and implement docx/odt parsing (e.g., extract text) instead of treating as plain UTF-8 text.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| config.getOptionalString( | ||
| 'lightspeed.aiNotebooks.queryDefaults.provider_id', | ||
| ) ?? ''; | ||
|
|
Member
Author
There was a problem hiding this comment.
@JslYoon You were not reading the default models so I have added these code, let me know it looks good to you, or I can remove it.
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.
Description
Depends on this #2704 , this should be merged only after #2704 got merged.
Depends on this backend PR too. https://github.com/redhat-developer/rhdh-plugins/pull/2742/changes
Fixed
Configuration
Add config in the app-config
UI after changes
https://github.com/user-attachments/assets/85fa5f26-cfcb-4b5a-8aea-4a8e5e8ab042
https://github.com/user-attachments/assets/d6beaf03-1151-4baa-8ee9-c650237a9fd7
✔️ Checklist