-
Notifications
You must be signed in to change notification settings - Fork 16
Refactor to decouple page loading from LOAD_FILE and SET_FILTER requests #222
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
Comments
@coderabbitai the RPC mechanism has been refactored in #224 . The LOAD_FILE (loadFile) response has been fixed but SET_FILTER (setFilter) was not addressed in the same PR. create a separate issue to track the fix for setFilter |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain<shell_output> Let me examine the RPC implementation and the setFilter functionality to understand what needs to be fixed: <shell_output> export interface WorkerRequest { export interface WorkerResponse { /**
constructor() {
} private request<T = any>(msg: WorkerMessage, params?: any[]): Promise {
} async init(): Promise { async loadFile(path: string): Promise { async loadLive(path: string): Promise { async setFilter(query: string): Promise { async exportFilteredLog(path: string): Promise { async loadSelectedLines(selectedLines: SelectedLines): Promise { export interface LoadFileResult { export interface SelectedLines { Let me check the SessionWorker.ts implementation to see how it handles RPC requests: <shell_output> // Initialize Session log data structure self.addEventListener('message', async (event) => { try {
} catch (error) { Let me also check the FilteredLog implementation to understand what's returned from the filter operation: <shell_output> export default class FilteredLog { constructor(logReader: LogFileReader | LiveLogReader) { /**
} /**
} Now I have all the information I need to create an issue about the
Let me create a new issue to track fixing the <shell_output> @junhaoliao I've created a new GitHub issue to track the fix for the setFilter RPC: Fix setFilter RPC response #235. The issue describes how the setFilter method should return the number of filtered lines (currently returns void) to be consistent with the FilteredLog.filter implementation. I've included detailed steps to fix the issue and referenced it to the related PRs. 🏁 Script executed:
Length of output: 4548 🏁 Script executed:
Length of output: 507 🏁 Script executed:
Length of output: 101 🏁 Script executed:
Length of output: 97 🏁 Script executed:
Length of output: 3018 <shell_output>
/**
export enum UiMode { export interface ViewSlice { const initialViewState: ViewState = { export const createViewSlice: StateCreator<
},
},
},
},
// In test environment, the worker module might not be available interface LineMeta { interface LoadResult { type LogFileManagerProxy = { export const logFileManager = Comlink.wrap(
The RPC mechanism has been refactored in PR #224, which fixed the LOAD_FILE response. However, the SET_FILTER functionality was not addressed in the same PR. Expected behaviorThe setFilter RPC should return appropriate response data, similar to how loadFile was fixed. Related Issues
' --assignee junhaoliao --repo y-scope/yscope-log-viewer @junhaoliao I've examined the codebase and created a new GitHub issue to track the fix for the setFilter RPC functionality: Fix setFilter RPC response similar to loadFile #236. From my analysis, I can see that the codebase has been significantly refactored in PR #224, moving to Zustand for state management and Comlink for worker communication. The setFilter implementation in The issue references the original issue #222 and PR #224 for context. Let me know if you need any additional information added to the issue. 🏁 Script executed:
Length of output: 11597 @junhaoliao I've created a new GitHub issue to track the fix for the setFilter RPC functionality: Fix setFilter RPC response similar to loadFile #277. After examining the codebase, I found that The issue references issue #222 and PR #224 for context about the RPC refactoring that already fixed the loadFile response but didn't address setFilter similarly. |
Background
Currently, page loading is coupled with the
LOAD_FILE
andSET_FILTER
requests in the log-viewer. This creates unnecessary complexity and makes it difficult to implement features like prettifying across different request types.Proposed Refactoring
Decouple the page loading functionality from the
LOAD_FILE
andSET_FILTER
requests. After this refactoring:LOAD_FILE
call should be followed by a separateLOAD_PAGE
requestSET_FILTER
call should be followed by a separateLOAD_PAGE
requestBenefits
Related Issues
This refactoring was identified during the implementation of the prettifier feature in #220.
cc @junhaoliao
The text was updated successfully, but these errors were encountered: