feat(new-webui): Add JSON syntax highlighting for clp-s query results; Configure syntax based on storage engine.#973
Conversation
WalkthroughThe changes remove the "highlight.js" dependency from the client package and update the Changes
Sequence Diagram(s)sequenceDiagram
participant Config
participant MessageComponent
participant SyntaxHighlighter
MessageComponent->>Config: Import CLP_STORAGE_ENGINES, SETTINGS_STORAGE_ENGINE
MessageComponent->>MessageComponent: Determine language (json or armasm)
MessageComponent->>SyntaxHighlighter: Render with 'tomorrow' style and selected language
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
components/log-viewer-webui/client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
components/log-viewer-webui/client/package.json(0 hunks)components/log-viewer-webui/client/src/config/index.ts(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/index.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- components/log-viewer-webui/client/package.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/index.tsxcomponents/log-viewer-webui/client/src/config/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (2)
components/log-viewer-webui/client/src/config/index.ts (1)
17-19: 🧹 Nitpick (assertive)Consider using STREAM_TYPE constant for better readability.
The conditional logic is correct but could be more readable by referencing the STREAM_TYPE constant that's derived from this logic.
-const STREAM_TYPE = CLP_STORAGE_ENGINES.CLP === SETTINGS_STORAGE_ENGINE ? - "ir" : - "json"; +const STREAM_TYPE = SETTINGS_STORAGE_ENGINE === CLP_STORAGE_ENGINES.CLP ? "ir" : "json";Likely an incorrect or invalid review comment.
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/index.tsx (1)
2-2: LGTM! Good improvement to syntax highlighting.The change from a hardcoded style to the
qtcreatorLighttheme from react-syntax-highlighter is a good improvement that removes the dependency on highlight.js while maintaining consistent styling.Also applies to: 45-45
| CLP_S = "clp-s", | ||
| } | ||
|
|
||
| const SETTINGS_STORAGE_ENGINE = settings.ClpStorageEngine as CLP_STORAGE_ENGINES; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding runtime validation for the type assertion.
The type assertion as CLP_STORAGE_ENGINES assumes that settings.ClpStorageEngine will always be a valid enum value, but this could cause runtime issues if the settings file contains an unexpected value.
-const SETTINGS_STORAGE_ENGINE = settings.ClpStorageEngine as CLP_STORAGE_ENGINES;
+const SETTINGS_STORAGE_ENGINE = Object.values(CLP_STORAGE_ENGINES).includes(settings.ClpStorageEngine as CLP_STORAGE_ENGINES)
+ ? settings.ClpStorageEngine as CLP_STORAGE_ENGINES
+ : CLP_STORAGE_ENGINES.CLP; // Default fallback📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const SETTINGS_STORAGE_ENGINE = settings.ClpStorageEngine as CLP_STORAGE_ENGINES; | |
| -const SETTINGS_STORAGE_ENGINE = settings.ClpStorageEngine as CLP_STORAGE_ENGINES; | |
| +const SETTINGS_STORAGE_ENGINE = Object.values(CLP_STORAGE_ENGINES).includes(settings.ClpStorageEngine as CLP_STORAGE_ENGINES) | |
| + ? settings.ClpStorageEngine as CLP_STORAGE_ENGINES | |
| + : CLP_STORAGE_ENGINES.CLP; // Default fallback |
🤖 Prompt for AI Agents
In components/log-viewer-webui/client/src/config/index.ts at line 12, the code
uses a type assertion to cast settings.ClpStorageEngine to CLP_STORAGE_ENGINES
without runtime validation, which can cause issues if the value is invalid. Add
a runtime check to verify that settings.ClpStorageEngine is a valid member of
the CLP_STORAGE_ENGINES enum before assigning it to SETTINGS_STORAGE_ENGINE, and
handle invalid values appropriately, such as by throwing an error or falling
back to a default.
| language={ | ||
| CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE ? | ||
| "json" : | ||
| "armasm" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider documenting the choice of "armasm" as the default language.
The conditional logic correctly selects "json" for CLP_S, but the choice of "armasm" as the default language for CLP seems unconventional for log data. Consider adding a comment explaining this choice or evaluating if a more generic language like "text" would be more appropriate.
language={
+ // Use JSON highlighting for CLP_S, armasm for CLP intermediate representation
CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE ?
"json" :
"armasm"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| language={ | |
| CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE ? | |
| "json" : | |
| "armasm" | |
| } | |
| language={ | |
| // Use JSON highlighting for CLP_S, armasm for CLP intermediate representation | |
| CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE ? | |
| "json" : | |
| "armasm" | |
| } |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/index.tsx
around lines 47 to 51, add a comment explaining why "armasm" is chosen as the
default language when SETTINGS_STORAGE_ENGINE is not CLP_S. This clarifies the
rationale behind using "armasm" for log data and helps future maintainers
understand the decision. Alternatively, evaluate if using a more generic
language like "text" would be more suitable and update the code accordingly.
| import { | ||
| CLP_STORAGE_ENGINES, | ||
| SETTINGS_STORAGE_ENGINE, | ||
| } from "../.././../../../config"; |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify the import path for the config module.
The import path ../.././../../../config appears to have an unusual number of relative directory navigations. Please verify this resolves to the correct config module location.
🏁 Script executed:
#!/bin/bash
# Description: Verify the config module import path is correct
# Expected: The path should resolve to components/log-viewer-webui/client/src/config/index.ts
# Find the config file location
fd "index.ts" --path "**/config/**"
# Check the actual file structure to verify the relative path
fd "index.tsx" --path "**/Message/**" --exec dirname {} | head -1 | xargs -I {} bash -c 'echo "Message component location: {}" && echo "Relative path from Message to config:" && realpath --relative-to="{}" "components/log-viewer-webui/client/src/config"'Length of output: 726
Import path is valid but simplify the redundant segment
The import
import {
CLP_STORAGE_ENGINES,
SETTINGS_STORAGE_ENGINE,
} from "../.././../../../config";correctly resolves to components/log-viewer-webui/client/src/config/index.ts, but the ./ is unnecessary. Please update it to:
-import {
- CLP_STORAGE_ENGINES,
- SETTINGS_STORAGE_ENGINE,
-} from "../.././../../../config";
+import {
+ CLP_STORAGE_ENGINES,
+ SETTINGS_STORAGE_ENGINE,
+} from "../../../../../config";This small change improves readability without affecting functionality.
🤖 Prompt for AI Agents
In
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/index.tsx
around lines 6 to 9, the import path for the config module includes an
unnecessary './' segment in "../.././../../../config". Remove the redundant './'
so the import path becomes "../../../../../config" to simplify and improve
readability without changing functionality.
junhaoliao
left a comment
There was a problem hiding this comment.
i believe this depends on #967 so let's merge 967 first
For the PR title, how about:
feat(new-webui): Add JSON syntax highlighting for clp-s query results; Configure syntax based on storage engine.
| language={ | ||
| CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE ? | ||
| "json" : | ||
| "armasm" |
There was a problem hiding this comment.
Out of scope of this PR - We should consider adding a custom language instead of reusing armasm since armasm is not directly related to logging
@coderabbitai submit an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao I've created a GitHub issue to track the suggestion about using a custom language for syntax highlighting instead of reusing armasm for logging data. The issue includes the context from this PR and discussion, and has been assigned to you as requested.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track the custom language improvement
gh issue create \
--title "Consider adding custom language for syntax highlighting instead of reusing armasm" \
--body "Currently, the syntax highlighter uses \`armasm\` as a fallback language for non-JSON log data, but \`armasm\` (ARM assembly) is not directly related to logging and may not provide optimal highlighting for log messages.
**Current behavior:**
- When \`CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE\`, JSON syntax highlighting is used
- Otherwise, \`armasm\` is used as the fallback language
**Suggested improvement:**
Consider implementing a custom language definition specifically tailored for log data or use a more appropriate existing language that better matches the structure of log messages.
**References:**
- Original PR: https://github.com/y-scope/clp/pull/973
- Discussion: https://github.com/y-scope/clp/pull/973#discussion_r2136170754
**Requested by:** @junhaoliao" \
--assignee junhaoliao
Length of output: 962
@junhaoliao Perfect! I've successfully created GitHub issue #989 to track the suggestion about using a custom language for syntax highlighting instead of reusing armasm.
The issue has been:
- ✅ Created with a descriptive title and detailed explanation
- ✅ Assigned to you as requested
- ✅ Includes references to this PR and discussion thread
- ✅ Explains the current behavior and suggested improvement
You can view the issue at: #989
This will help track the improvement for future implementation while keeping it separate from the current PR scope.
🧬 Code Graph Analysis Results
Possibly relevant snippets from components/log-viewer-webui/client/src/config/index.ts
At line range 22-23:
CLP_STORAGE_ENGINES
SETTINGS_STORAGE_ENGINEThese constants are imported and used in the Message component to determine the syntax highlighting language. Specifically, the code checks if CLP_STORAGE_ENGINES.CLP_S equals SETTINGS_STORAGE_ENGINE to decide whether to use "json" or "armasm" as the language for syntax highlighting.
Possibly relevant snippet from components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/utils.ts
At line range 5-11:
export const highlighterCustomStyles: React.CSSProperties = {
background: "none",
border: "none",
fontFamily: "inherit",
margin: "0",
padding: "0",
};This object defines custom CSS styles applied to the SyntaxHighlighter component in the Message component to control its appearance, ensuring it inherits font styles and has no extra padding or borders.
These snippets provide context on the configuration constants controlling the syntax highlighting language choice and the styling applied to the syntax highlighter in the Message component.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
switch to "tomorrow" per @kirkrodrigues |
junhaoliao
left a comment
There was a problem hiding this comment.
any thoughts on the previously proposed title?
feat(new-webui): Add JSON syntax highlighting for clp-s query results; Configure syntax based on storage engine.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/index.tsx (1)
43-55: 🧹 Nitpick (assertive)Lazy-load heavy syntax highlighter
react-syntax-highlightercan bloat the bundle. Consider dynamic import orReact.lazyfor theSyntaxHighlighterso it’s only loaded when needed.
♻️ Duplicate comments (2)
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/index.tsx (2)
6-9: Remove redundant segment in import path
The./in"../.././../../../config"is unnecessary. Simplify the import to improve readability without affecting functionality:-import { - CLP_STORAGE_ENGINES, - SETTINGS_STORAGE_ENGINE, -} from "../.././../../../config"; +import { + CLP_STORAGE_ENGINES, + SETTINGS_STORAGE_ENGINE, +} from "../../../../../config";
47-51: Document fallback language choice
Falling back to"armasm"for non-JSON logs may confuse future maintainers. Add a comment explaining this choice or consider a more generic language like"text".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/index.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/index.tsx
🧬 Code Graph Analysis (1)
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/index.tsx (1)
components/log-viewer-webui/client/src/config/index.ts (2)
CLP_STORAGE_ENGINES(22-22)SETTINGS_STORAGE_ENGINE(23-23)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (1)
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/index.tsx (1)
45-46: Verify default inline styling
By droppinguseInlineStyles={false}, the highlighter now applies inline styles by default. Ensure this doesn’t conflict with your global CSS or AntD theming.
| @@ -1,12 +1,15 @@ | |||
| import SyntaxHighlighter from "react-syntax-highlighter"; | |||
| import {tomorrow} from "react-syntax-highlighter/dist/esm/styles/hljs"; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider making the theme configurable
Hardcoding the tomorrow style ties the component to one theme. To support future theming (e.g. dark mode), consider passing the style as a prop or using a context/provider.
🤖 Prompt for AI Agents
In
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Message/index.tsx
at line 2, the import of the hardcoded 'tomorrow' theme from
react-syntax-highlighter limits theming flexibility. Refactor the component to
accept the syntax highlighter style as a prop or obtain it from a
context/provider, removing the direct import of 'tomorrow'. This will enable
dynamic theming such as dark mode in the future.
…; Configure syntax based on storage engine. (y-scope#973) Co-authored-by: Marco <david.marcovitch@yscope.com>
This PR should be merged after #967, since there is new support for determining clp-s vs. clp in that PR
Description
PR adds syntax highlighting for JSON. Before was specific for unstructured.
I also removed highlight js dependency and just switched to a theme supported by react syntax highligher.
Checklist
breaking change.
Validation performed
Highlighted as json
Summary by CodeRabbit