Skip to content

Improve readability by formatting code within brackets. #220

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

Closed
junhaoliao opened this issue Mar 30, 2025 · 8 comments
Closed

Improve readability by formatting code within brackets. #220

junhaoliao opened this issue Mar 30, 2025 · 8 comments
Labels
enhancement New feature or request

Comments

@junhaoliao
Copy link
Member

Request

Add functionality to automatically format and indent code or JSON-like structures enclosed within brackets {} or [] in log messages. This would significantly improve readability and facilitate faster debugging and analysis.

Possible implementation

  1. Introduce a "Prettify" toggle button in the right corner of the status bar of the log-viewing interface. Users can easily switch formatting on or off as needed.
  2. Support toggling formatting through a URL hash parameter, enabling easy sharing of pre-formatted logs.
  3. Implement a lightweight formatting solution, either through a custom-built formatter or by integrating an existing solution like js-beautify, to automatically format and indent structured data within each log entry upon toggling.
@junhaoliao junhaoliao added the enhancement New feature or request label Mar 30, 2025
@junhaoliao
Copy link
Member Author

junhaoliao commented Mar 30, 2025

Example code using js-beautify:

import beautify from 'js-beautify';


console.log(
    beautify.js("INFO some.class some.module.file.js:123 some message {123} someFunc(123,123) {return 1;}")
)

->

INFO some.class some.module.file.js: 123 some message {
    123
}
someFunc(123, 123) {
    return 1;
}

@zzxthehappiest
Copy link
Contributor

zzxthehappiest commented Mar 31, 2025

Hi @junhaoliao , I have started looking into this. But I find I am still not familar with React framework enough, so it took longer time than I thought :(

Image

Image

I feel the js-beautify is a bit ugly after reformatting the log messages. Do you have any comments on its looking? I found that it would also reformat the timestamp strings.

I am also thinking of some problems:

  • After reformatting, I think it maybe hard for users to determine which one is which one, espectially when the log message is huge. Maybe we need to change the line number logic as well, for example, change the line number to log message index?

  • Also I found that the url of openning a local file is http://localhost:3010/#logEventNum=12, I think this link is not accessible if sharing it with others.

  • I also wonder which task is of higher priority for now (this one or clp-ffi-js API change). I heard from wxc that thing now is of highest priority, so I look into this. But I feel it might take some more time to polish this.

@junhaoliao
Copy link
Member Author

@zzxthehappiest thanks for looking into this promptly!

Also I found that the url of openning a local file is http://localhost:3010/#logEventNum=12, I think this link is not accessible if sharing it with others.

Right, this is expected. We currently don't really cache the last opened file in the log-viewer, so it is expected that whenever user refreshes the browser page, the last opened file is lost. That said, I believe it is possible with the latest web standards (Origin private file system or IndexedDB) @coderabbitai can you help create a feature request to track this?

I also wonder which task is of higher priority for now (this one or clp-ffi-js API change). I heard from wxc that thing now is of highest priority, so I look into this. But I feel it might take some more time to polish this.

Getting this prettifier feature done is actually more important for us because it helps us to reach feature parity with a private deprecated edition of the log-viewer (see below). The log level filter, though broken for Python logs, has already had some great improvements than the one in the deprecated edition. Let's get #220 done first if you may.


@kirkrodrigues and I had an offline discussion about this feature request. In fact, we previously had an implementation of a simple formatter in a private branch: https://github.com/junhaoliao/yscope-log-viewer/blob/beta-release/src/Viewer/services/decoder/SimplePrettifier.js because js-beautify was a bit slow. (I just confirmed again with the latest version js-beautify and still found it ~30x slower than our implementation.

Let's split the code changes into two PRs so it becomes easier for us all to see progresses:

  1. Provide the UI and URL interfaces for enabling this feature. For the formatter, we can use js-beautify for now.
    a. Let's add a new option isPrettified in the LOAD_PAGE protocol and update the loadPage() function in the StateContextProvider to accept a new parameter for this:
    cursor: CursorType;
    };

    b. Let's add isPrettified as a boolean "hash" URL parameter in the UrlContextProvider utilities. (We might not need to make any code changes for this item because
    const getUpdatedHashParams = (updates: UrlHashParamUpdatesType) => {
    may have handled it well. Feel free to post any questions for any doubts once encountered on this. )
    c. In the StateContextProvider (essentially a controller for talking to the MainWorker), we can subscribe to this state from the UrlContextProvider with a useEffect hook of dependency [isPrettified], to invoke loadPage() whenever the isPrettified URL hash parameter is changed. (For importing a state from the UrlContextProvider, you can use const {isPrettified} = useContext(UrlContextProvider); in the first few lines of the StateContextProvider.)
    d. Add a button in the right of the status bar to toggle the isPrettified URL hash parameter. (Call updateWindowUrlHashParams())
  2. Port the SimplePrettifier as a utility function (instead of a class) into utils/prettifier.ts.
    a. Replace the js-beautify formatting with the ported utility function.
    b. Add test cases for the function.

Feel free to share your thoughts on those.

Copy link

coderabbitai bot commented Mar 31, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@junhaoliao I've created a feature request for caching the last opened file in the log-viewer. This would allow the application to persist files between browser refreshes using modern web APIs like Origin Private File System or IndexedDB.

Now regarding the prettifier feature, your implementation plan looks well-structured:

  1. First PR for UI and URL interfaces using js-beautify temporarily
  2. Second PR to replace with your optimized SimplePrettifier implementation

The performance difference (30x faster with your implementation) is significant and definitely justifies this approach. Breaking it into two PRs will make the changes more manageable and easier to review.


🏁 Script executed:

#!/bin/bash
# Create a GitHub issue to track the feature request
gh issue create \
  --title "Cache last opened file in log-viewer for persistence" \
  --body "## Background

Currently, the log-viewer doesn't cache the last opened file, so whenever a user refreshes the browser page, the last opened file is lost. This makes sharing links to local files impractical (e.g., \`http://localhost:3010/#logEventNum=12\`) as they are not accessible across sessions.

## Feature Request

Implement functionality to cache and restore the last opened file when the log-viewer is refreshed or reopened.

## Proposed Implementation

Utilize modern web standards to store file references persistently:
- [Origin private file system](https://developer.mozilla.org/en-US/docs/Web/API/File_System_API#origin_private_file_system)
- [IndexedDB](https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API)

These APIs would allow storing the file reference and related viewing state (such as scroll position, filters, etc.) across browser sessions.

## Related Discussion
This request originated from a discussion in #220.

cc @junhaoliao" \
  --repo "y-scope/yscope-log-viewer" \
  --assignee "junhaoliao"

Length of output: 1166


@junhaoliao I've created a feature request for caching the last opened file in the log-viewer at issue #221. This will track the implementation of file persistence between browser sessions.

Regarding your prettifier implementation plan, I think splitting it into two PRs is an excellent approach:

  1. First implementing the UI/URL interfaces with js-beautify temporarily
  2. Then replacing it with your optimized SimplePrettifier

The 30x performance improvement with your custom implementation definitely justifies this two-step approach. Breaking the work into these logical stages will make the changes more manageable while still delivering the functionality quickly. The step-by-step instructions you've provided are clear and comprehensive, making it easy for the developer to implement this feature effectively.

@zzxthehappiest
Copy link
Contributor

Thank you very much! This is much clearer now, and thanks for pointing me where to write the code (because I was heavily using Ctrl&F to search keywords to add the code, I will try my best to get myself familar with React and the project's code base as soon as possible!)

@zzxthehappiest
Copy link
Contributor

zzxthehappiest commented Apr 1, 2025

It took some time on debugging React's hook ˃̣̣̥᷄⌓˂̣̣̥᷅ (I will proactively learn more basics). Anyways, I am finishing the implementatione of the first task and about to write unit tests. In terms of the unit tests, could you @junhaoliao give some tips on what should I focus on :)

PS: I also have a question, when loading file, if the URL contains isPrettified=true do we need show prettified content accordingly? Because I think you only mentioned LOAD_PAGE. Actually I also modified the SET_FILTER to make it support the prettifying as well, because it is possible that user uses both log filter and prettifying at the same time. Let me know if you just want it affects LOAD_PAGE!

@junhaoliao
Copy link
Member Author

It took some time on debugging React's hook ˃̣̣̥᷄⌓˂̣̣̥᷅ (I will proactively learn more basics).

No worries. Some code was checked-in in a very rush manner, and the fact that you managed to understand it without any assistance is truly remarkable. (We should write better docs in the first place. )

Anyways, I am finishing the implementatione of the first task and about to write unit tests. In terms of the unit tests, could you @junhaoliao give some tips on what should I focus on :)

Though non-ideal, we only set up unit test cases for the utility functions in src/utils (, and only a few of those files have coverage enforcement). I believe we will be able to set up unit tests more easily once the refactoring in #211 is finished; before then, it is fine to omit test cases addition for such UI changes. That said, let's try to add a manual testing item at https://github.com/y-scope/yscope-log-viewer/blob/main/docs/src/dev-guide/contributing-validation.md#manual-testing . Something like

* Toggling "Prettify" in both the status bar and the address bar.

is sufficient.

When adding the SimplePrettifier (the 2nd PR), some basic test cases will definitely be beneficial and appreciated.

when loading file, if the URL contains isPrettified=true do we need show prettified content accordingly? Because I think you only mentioned LOAD_PAGE. Actually I also modified the SET_FILTER to make it support the prettifying as well, because it is possible that user uses both log filter and prettifying at the same time.

Good catch! Yes, we want to change LOAD_FILE as well. In fact, this can be particularly useful if the log-viewer link is generated. If a customer wishes to integrate their application with the log viewer, they can generate a link using their desired ?fileSrc, #logEventNum, and #isPrettified parameters. With the link, the log viewer will initialize with the specified file, cursor position, and formatting state. Let's add this option to all request types whose responses include PAGE_DATA, namely LOAD_FILE, LOAD_PAGE, and SET_FILTER.


@coderabbitai Let's create an issue to track the refactoring effort to decouple page loading from the LOAD_FILE and SET_FILTER requests. After the refactoring, any existing LOAD_FILE or SET_FILTER call should be followed by a separate LOAD_PAGE request.

Copy link

coderabbitai bot commented Apr 1, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

The key task is to create an issue for tracking the refactoring effort to decouple page loading as requested. I'll create this issue now:

@junhaoliao I've created an issue to track the refactoring effort as requested. The issue focuses on decoupling page loading from the LOAD_FILE and SET_FILTER requests, which will make the codebase more maintainable and easier to extend with features like prettifying.

The refactoring will ensure that any existing LOAD_FILE or SET_FILTER calls will be followed by separate LOAD_PAGE requests, creating a cleaner separation of concerns in the codebase.

You can find the new issue in the repository's issue tracker. Let me know if you need any additional information or adjustments to the issue description.


🏁 Script executed:

#!/bin/bash
# Let's look at the relevant files to understand the structure
# Check the worker.ts file that was mentioned
fd "worker.ts" --type f -X cat

Length of output: 9540


🏁 Script executed:

#!/bin/bash
# Create a GitHub issue to track the refactoring effort
gh issue create \
  --title "Refactor to decouple page loading from LOAD_FILE and SET_FILTER requests" \
  --body "## Background
Currently, page loading is coupled with the \`LOAD_FILE\` and \`SET_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\` and \`SET_FILTER\` requests. After this refactoring:
- Any existing \`LOAD_FILE\` call should be followed by a separate \`LOAD_PAGE\` request
- Any existing \`SET_FILTER\` call should be followed by a separate \`LOAD_PAGE\` request

## Benefits
- Cleaner separation of concerns
- More maintainable codebase
- Easier to implement features that affect page rendering (like prettifying)
- Consistent behavior across different loading scenarios

## Related Issues
This refactoring was identified during the implementation of the prettifier feature in #220.

cc @junhaoliao" \
  --repo "y-scope/yscope-log-viewer" \
  --assignee "junhaoliao"

Length of output: 1110


@junhaoliao I've created an issue to track the refactoring effort as requested. The issue has been created as #222 in the yscope-log-viewer repository.

The issue details the proposed refactoring to decouple page loading from the LOAD_FILE and SET_FILTER requests, which will create a cleaner separation of concerns and make it easier to implement features like prettifying across different loading scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants