-
Notifications
You must be signed in to change notification settings - Fork 17
fix(QueryEditor): pass base64 from settings #3093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the handling of the base64 encoding setting by passing it explicitly as a parameter from the QueryEditor component instead of having the API layer directly access the settings manager. The refactoring moves the responsibility of reading the BINARY_DATA_IN_PLAIN_TEXT_DISPLAY setting to the component level using the useSetting hook, which aligns with React best practices.
Key changes:
- QueryEditor now uses
useSettinghook to read the binary data display setting and calculatesencodeTextWithBase64 - The
base64value is now passed explicitly to query API calls (sendQueryandstreamQuery) - API services (
viewer.tsandstreaming.ts) no longer directly accesssettingsManager, receivingbase64from parameters instead
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/api/query.ts | Added optional base64 parameter to SendQueryParams interface |
| src/store/reducers/query/query.ts | Added base64 parameter to internal query param types and passed it through to API calls |
| src/services/api/viewer.ts | Removed direct settingsManager access, now uses base64 from params; also replaced settingsManager with readSettingValueFromLS for tracing |
| src/services/api/streaming.ts | Removed direct settingsManager access, now uses base64 from params; replaced settingsManager with readSettingValueFromLS for tracing |
| src/services/api/base.ts | Replaced settingsManager with readSettingValueFromLS for tracing flag |
| src/containers/Tenant/Query/QueryEditor/QueryEditor.tsx | Added useSetting hook to read binary data display setting and passes calculated base64 value to query mutations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6 files reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
| const enableTracing = settingsManager.readUserSettingsValue( | ||
| DEV_ENABLE_TRACING_FOR_ALL_REQUESTS, | ||
| ); | ||
| const enableTracing = readSettingValueFromLS(DEV_ENABLE_TRACING_FOR_ALL_REQUESTS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we have two options to get setting value - via readSettingValueFromLS or via useSetting
Now its not that obvious (for me ofc) what method when to use.
What do you think of it? Could useSetting consider itself where to take setting value from?
Maybe its a bad idea - just a proposal for consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR: currently for all settings you should use useSetting from utils/hooks.
Tracing is a dev setting, it should never be synced with external storage. So I read it directly from localStorage.
All other settings should be used and manipulated with useSetting, it will use local storage or meta settings service depending on app configuration and user auth.
In this PR I use old useSetting, new approach breaks tests, so I try separate all refactoring to reduce places where things can go wrong. Probably I should have named new hook differently to reduce ambiguity, but I had false hope that it will work from the beginning and I could just remove old code. So now we have two useSetting hook, you should use old hook from utils while new useSetting is not proved to work properly in all cases
Part of #2892
Stand: https://nda.ya.ru/t/BEIjuuLI7N8Qd7
Goal: use
useSettingfor base64 value instead of direct access in apiCI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: ✅
Current: 66.11 MB | Main: 66.11 MB
Diff: +0.41 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information
Greptile Summary
useSettinghook, moving the responsibility from API layer to the QueryEditor componentImportant Files Changed
useSettinghook and passes value to all query operationsbase64parameter toSendQueryParamsinterface for explicit encoding controlConfidence score: 5/5
Sequence Diagram
sequenceDiagram participant User participant QueryEditor participant queryApi participant StreamingAPI participant ViewerAPI participant Redux User->>QueryEditor: "Click Execute" QueryEditor->>Redux: "setQueryResult (loading)" alt Streaming Enabled QueryEditor->>queryApi: "streamQuery" queryApi->>StreamingAPI: "streamQuery" StreamingAPI->>Redux: "setStreamSession" loop Data Chunks StreamingAPI->>Redux: "addStreamingChunks" end StreamingAPI->>Redux: "setStreamQueryResponse" else Regular Query QueryEditor->>queryApi: "sendQuery" queryApi->>ViewerAPI: "sendQuery" ViewerAPI->>Redux: "setQueryResult (with data)" queryApi->>Redux: "updateQueryInHistory" end QueryEditor->>Redux: "saveQueryToHistory" Redux->>User: "Display Results"Context used:
dashboard- description of repository for agents (source)