Skip to content

Conversation

@artemmufazalov
Copy link
Member

@artemmufazalov artemmufazalov commented Nov 17, 2025

Part of #2892

Stand: https://nda.ya.ru/t/BEIjuuLI7N8Qd7

Goal: use useSetting for base64 value instead of direct access in api

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
378 374 0 2 2
Test Changes Summary ⏭️2

⏭️ Skipped Tests (2)

  1. Scroll to row, get shareable link, navigate to URL and verify row is scrolled into view (tenant/diagnostics/tabs/queries.test.ts)
  2. Copy result button copies to clipboard (tenant/queryEditor/queryEditor.test.ts)

Bundle Size: ✅

Current: 66.11 MB | Main: 66.11 MB
Diff: +0.41 KB (0.00%)

✅ Bundle size unchanged.

ℹ️ CI Information
  • Test recordings for failed tests are available in the full report.
  • Bundle size is measured for the entire 'dist' directory.
  • 📊 indicates links to detailed reports.
  • 🔺 indicates increase, 🔽 decrease, and ✅ no change in bundle size.

Greptile Summary

  • Refactors base64 encoding configuration from direct localStorage access to centralized settings using useSetting hook, moving the responsibility from API layer to the QueryEditor component
  • Updates query-related interfaces and API methods to accept base64 parameter as an optional boolean value, enabling explicit control over binary data encoding preferences
  • Replaces hardcoded base64 logic in API classes with parameter-based approach, improving testability and reducing coupling between settings management and API services

Important Files Changed

Filename Overview
src/containers/Tenant/Query/QueryEditor/QueryEditor.tsx Implements centralized base64 setting using useSetting hook and passes value to all query operations
src/services/api/viewer.ts Removes direct settings access and accepts base64 parameter from caller instead
src/types/api/query.ts Adds optional base64 parameter to SendQueryParams interface for explicit encoding control

Confidence score: 5/5

  • This PR is safe to merge with minimal risk of introducing bugs or breaking changes
  • Score reflects clean architectural refactoring with backward compatibility maintained through optional parameters
  • No files require special attention as the changes follow established patterns and maintain consistency

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"
Loading

Context used:

  • Context from dashboard - description of repository for agents (source)

Copy link
Contributor

Copilot AI left a 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 useSetting hook to read the binary data display setting and calculates encodeTextWithBase64
  • The base64 value is now passed explicitly to query API calls (sendQuery and streamQuery)
  • API services (viewer.ts and streaming.ts) no longer directly access settingsManager, receiving base64 from 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

@artemmufazalov artemmufazalov marked this pull request as ready for review November 17, 2025 18:21
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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);
Copy link
Collaborator

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.

Copy link
Member Author

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

@artemmufazalov artemmufazalov changed the title refactor(QueryEditor): pass base64 from settings fix(QueryEditor): pass base64 from settings Nov 18, 2025
@artemmufazalov artemmufazalov added this pull request to the merge queue Nov 18, 2025
Merged via the queue into main with commit eb54851 Nov 18, 2025
17 checks passed
@artemmufazalov artemmufazalov deleted the base-64 branch November 18, 2025 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants