feat(formatter): Display entire log event as JSON by default and remind users to set format string. #129
Conversation
WalkthroughThe pull request introduces several modifications across various components and files. Key changes include the transition of state management for the settings modal to a context-based approach in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
src/typings/notifications.ts (1)
4-10: Consider adding error handling to the callback typeThe type definition is well-structured, but consider making the callback more robust by handling potential errors.
type PopUpButton = { title: string, - callback: () => void, + callback: () => void | Promise<void>, }src/components/PopUps/index.css (1)
33-35: Consider avoiding the use of!importantWhile the new CSS class follows the existing naming convention and serves its purpose for the support button, the use of
!importantmight make future style overrides difficult. Consider:
- Increasing specificity through class nesting
- Using CSS custom properties (variables) for margins
Here's an alternative approach:
-.pop-up-message-box-callback-button { - margin-top: 10px !important; -} +/* Define in :root */ +:root { + --ylv-popup-button-margin: 10px; +} + +/* Use more specific selector */ +.pop-up-message-box-alert .pop-up-message-box-callback-button { + margin-top: var(--ylv-popup-button-margin); +}src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx (1)
55-55: Consider adding error handling for undefined context.The transition from local state to context is a good architectural decision. However, consider adding error handling in case the context is accessed outside its provider:
- const {isSettingsModalOpen, setIsSettingsModalOpen} = useContext(StateContext); + const context = useContext(StateContext); + if (!context) { + throw new Error("SidebarTabs must be used within a StateContextProvider"); + } + const {isSettingsModalOpen, setIsSettingsModalOpen} = context;src/components/PopUps/PopUpMessageBox.tsx (2)
117-126: Consider accessibility and performance improvementsWhile the button implementation is functional, consider these enhancements:
- Add aria-label for screen readers
- Memoize the callback to prevent unnecessary re-renders
{button && ( <Button className={"pop-up-message-box-callback-button"} color={color} variant={"soft"} + aria-label={`Support action: ${button.title}`} onClick={button.callback} > {button.title} </Button> )}Also consider wrapping the component with React.memo and using useCallback for the onClick handler if the popup is frequently re-rendered.
Line range hint
48-73: Consider extracting dismiss logic to a custom hookThe auto-dismiss and manual dismiss logic could be extracted to a custom hook (e.g.,
useAutoDismiss) to improve code organization and reusability. This would separate the timing logic from the presentation logic.Example structure:
function useAutoDismiss(timeoutMillis: number, onDismiss: () => void) { const [percentRemaining, setPercentRemaining] = useState<number>(100); // ... existing timeout logic return { percentRemaining, handleDismiss }; }src/services/formatters/YscopeFormatter/index.ts (2)
41-44: Enhance documentation and add debug loggingWhile the implementation is correct, consider:
- Expanding the comment to explain why this is a special case and its purpose
- Adding debug logging when this fallback occurs to help track usage
- // Empty format string is special case where formatter returns all fields as JSON. + // When format string is empty, return all fields as JSON to help users + // discover available fields for formatting. This aids in format string configuration. if ("" === this.#processedFormatString) { + console.debug("Using JSON fallback due to empty format string"); return jsonValueToString(logEvent.fields); }
41-44: Document the empty format string architectureThe empty format string handling is split between the formatter (JSON output) and UI components (user notification). Consider adding a comment in the class JSDoc to document this architectural decision and the relationship between these components.
/** * A formatter that uses a YScope format string to format log events into a string. See * `YscopeFormatterOptionsType` for details about the format string. + * + * Special Cases: + * - Empty Format String: When the format string is empty, the formatter outputs all fields + * as JSON. This works in conjunction with UI components that notify users to configure + * their format string. */src/utils/config.ts (1)
22-22: Consider adding a migration strategy for existing usersWhile setting the default format string to empty aligns with the new feature, existing users might be surprised by the change in log output format. Consider:
- Adding a migration script to preserve existing user's format string
- Adding a clear notification about this change in the release notes
src/components/modals/SettingsModal/SettingsDialog.tsx (2)
41-42: Consider enhancing the helper text clarity and structureWhile the added text about empty string behaviour is accurate, consider these improvements for better user understanding:
- \`{@timestamp:timestamp:YYYY-MM-DD HH\\:mm\\:ss.SSS}\`. The default setting is - an empty string which will print all fields formatted as JSON.`, + \`{@timestamp:timestamp:YYYY-MM-DD HH\\:mm\\:ss.SSS}\`. + + The default setting is an empty string, which will: + - Display all available log fields in JSON format + - Help you identify field names for creating custom formats + + Example JSON output: {"@timestamp": "2024-01-01", "level": "INFO", ...}`,This restructuring:
- Separates the empty string behaviour from the format example
- Clarifies the purpose of showing all fields
- Provides a concrete example of the JSON output
Line range hint
27-56: Consider architectural improvements for configuration managementThe current implementation mixes configuration field definitions with their documentation. Consider these architectural improvements:
Move the configuration field definitions to a separate configuration module:
- Create
src/config/settings.tsfor field definitions- Include examples and documentation
- Enable reuse across different components
Create a documentation module for format string syntax:
- Centralize format string examples
- Make it available for both UI and documentation
- Easier to maintain and update
This separation would improve maintainability and make the configuration more reusable across the application.
src/contexts/StateContextProvider.tsx (1)
287-300: Consider enhancing the format string check and popup messageWhile the implementation works, there are several opportunities for improvement:
- The format string check could be more flexible to handle empty strings or other default-like values
- The popup message could provide more context about why formatting improves readability
- The timeout duration could be defined as a named constant
Consider these improvements:
+const FORMAT_STRING_POPUP_TIMEOUT = 2 * DEFAULT_AUTO_DISMISS_TIMEOUT_MILLIS; + if (getConfig(CONFIG_KEY.DECODER_OPTIONS).formatString === CONFIG_DEFAULT[CONFIG_KEY.DECODER_OPTIONS].formatString) { postPopUp({ button: { title: "Open Settings", callback: () => { setIsSettingsModalOpen(true); }, }, level: LOG_LEVEL.INFO, - message: "Input a custom format string in settings dialog" + - " to improve readability of JSON logs", + message: "Your logs are currently displayed in raw JSON format. " + + "Add a custom format string to extract and display specific fields " + + "in a more readable layout.", - timeoutMillis: 2 * DEFAULT_AUTO_DISMISS_TIMEOUT_MILLIS, + timeoutMillis: FORMAT_STRING_POPUP_TIMEOUT, title: "Format JSON Logs", }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx(3 hunks)src/components/PopUps/PopUpMessageBox.tsx(3 hunks)src/components/PopUps/index.css(1 hunks)src/components/modals/SettingsModal/SettingsDialog.tsx(1 hunks)src/contexts/StateContextProvider.tsx(10 hunks)src/services/formatters/YscopeFormatter/index.ts(2 hunks)src/typings/notifications.ts(2 hunks)src/utils/config.ts(2 hunks)
🔇 Additional comments (12)
src/typings/notifications.ts (1)
20-20: LGTM! Well-structured interface extension
The optional button property is a clean addition that maintains backward compatibility while enabling the new popup functionality described in the PR objectives.
src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx (2)
3-3: LGTM! Proper import organization.
The addition of useContext import is correctly placed with other React imports.
16-16: LGTM! Verify StateContext implementation.
The StateContext import is properly organized with other local imports.
Let's verify the StateContext implementation:
✅ Verification successful
StateContext implementation verified and provides required properties
The StateContext interface in src/contexts/StateContextProvider.tsx includes both required properties:
isSettingsModalOpen: booleansetIsSettingsModalOpen: (isOpen: boolean) => void
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that StateContext provides the required properties
# Check if StateContext provides isSettingsModalOpen and setIsSettingsModalOpen
ast-grep --pattern 'interface StateContextType {
$$$
isSettingsModalOpen: boolean
setIsSettingsModalOpen: $_
$$$
}'
Length of output: 125
Script:
#!/bin/bash
# Let's try a broader search to find the StateContext implementation and interface
# First, find the exact file path
fd "StateContextProvider" --type f
# Then examine the content of the context provider
rg -l "StateContext|StateContextType" --type ts --type tsx
# Look for any interface or type definition related to StateContext
ast-grep --pattern 'interface StateContextType {
$$$
}'
ast-grep --pattern 'type StateContextType = {
$$$
}'
# Look for the context creation
ast-grep --pattern 'export const StateContext = createContext<$_>($_)'
Length of output: 1972
src/components/PopUps/PopUpMessageBox.tsx (2)
11-11: LGTM! Import is properly organized
The Button import is correctly grouped with other @mui/joy components.
42-42: Verify type definitions for the button prop
Let's ensure the button prop is properly typed as optional in the PopUpMessage interface.
✅ Verification successful
The button prop is correctly typed as optional
The interface definition in src/typings/notifications.ts shows that button is properly typed as an optional property with button?: PopUpButton. The destructuring in PopUpMessageBox.tsx is consistent with this type definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PopUpMessage type definition
ast-grep --pattern 'interface PopUpMessage {
$$$
button?: {
$$$
}
$$$
}'
Length of output: 86
Script:
#!/bin/bash
# Let's try a broader search for PopUpMessage interface or type
ast-grep --pattern 'type PopUpMessage = {
$$$
}'
# Also search for interface definition
ast-grep --pattern 'interface PopUpMessage {
$$$
}'
# Backup search using ripgrep to find any PopUpMessage type definitions
rg "PopUpMessage" -A 10 -B 2
Length of output: 12852
src/services/formatters/YscopeFormatter/index.ts (2)
13-13: LGTM: Import addition is well-placed
The new utility import is properly grouped with related functions and follows the established naming convention.
41-44: Verify JSON conversion handling
Let's verify that jsonValueToString properly handles all possible field types and edge cases.
src/utils/config.ts (1)
Line range hint 22-47: Verify the impact of empty format string
Let's verify how these configuration values are used throughout the codebase to ensure there are no assumptions about non-empty format strings.
✅ Verification successful
Let me gather more specific information about how the formatString is used in the codebase.
Empty format string is handled properly and safe to use
The codebase explicitly handles empty format strings as a special case. In the YscopeFormatter implementation, an empty format string is treated as a specific feature that outputs all fields as JSON. This is documented in both the settings UI and implemented in the formatter.
Key findings:
- Empty format string is documented as the default setting in SettingsDialog.tsx
- YscopeFormatter has special handling for empty format strings
- No assumptions about non-empty format strings exist in the decoders or formatters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues with empty format string handling
# Find all files that reference formatString
echo "Files referencing formatString:"
rg -l "formatString"
# Check for potential null checks or empty string handling
echo "\nPotential places that might need empty string handling:"
rg -A 3 "formatString\s*[=!]==?\s*[\"']"
# Look for log formatting logic
echo "\nLog formatting implementation:"
ast-grep --pattern 'function $_($$_) {
$$$
formatString
$$$
}'
Length of output: 774
Script:
#!/bin/bash
# Check implementation details of formatString usage
# Look for formatString usage in decoders
echo "=== Decoder Implementation ==="
rg -C 5 "formatString" "src/services/decoders/"
# Check formatter implementation
echo "\n=== Formatter Implementation ==="
rg -C 5 "formatString" "src/services/formatters/"
# Check how formatString is handled in settings
echo "\n=== Settings Implementation ==="
rg -C 5 "formatString" "src/components/modals/SettingsModal/"
# Check type definitions
echo "\n=== Type Definitions ==="
cat src/typings/formatters.ts
Length of output: 13949
src/contexts/StateContextProvider.tsx (4)
64-65: LGTM: Interface changes are well-structured
The additions to the StateContextType interface are properly typed and follow React conventions for modal state management.
Also applies to: 78-78
91-91: LGTM: Default state is properly initialized
The default state for isSettingsModalOpen is appropriately set to false, maintaining consistency with typical modal behaviour.
247-248: LGTM: State hook implementation follows best practices
The useState hook for modal visibility is properly implemented with the default state value.
537-537: LGTM: Context value updates are consistent
The isSettingsModalOpen state and setter are properly exposed through the context provider, maintaining alphabetical ordering and following the established pattern.
Also applies to: 550-550
src/utils/config.ts
Outdated
| if ("" === value.timestampKey) { | ||
| result = "Timestamp key cannot be empty."; | ||
| } else if ("" === value.logLevelKey) { | ||
| result = "Log level key cannot be empty."; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve validation logic and user experience
The current validation implementation has several areas for improvement:
- The string comparison could be simplified using string length
- Consider validating formatString to distinguish between "intentionally empty" and "accidentally empty"
- Consider reordering validations to check all fields and return multiple errors
Here's a suggested improvement:
- if ("" === value.timestampKey) {
- result = "Timestamp key cannot be empty.";
- } else if ("" === value.logLevelKey) {
- result = "Log level key cannot be empty.";
+ const errors: string[] = [];
+ if (!value.timestampKey?.length) {
+ errors.push("Timestamp key cannot be empty");
+ }
+ if (!value.logLevelKey?.length) {
+ errors.push("Log level key cannot be empty");
+ }
+ if (!value.formatString?.length && !confirm("Are you sure you want to use empty format string? This will print all fields in JSON format.")) {
+ errors.push("Format string was left empty");
+ }
+ result = errors.length ? errors.join(". ") : null;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
This is not neccesary
|
This PR has been up 4 days without a review. What's the status? |
I've been dealing with customer issues in the past two business days and there was no one else triaging the review. I'll take a look once I wrap a customer meeting, if there's no new priorities identified, in an hour. |
There was a problem hiding this comment.
otherwise all good. we should probably only pop the message if the loaded log file is of type json / structuredIr but i'm fine leaving that to another PR. (if so, let's create an issue so we can keep track of it, after we get the file type checking done right. )
| level: LOG_LEVEL.INFO, | ||
| message: "Input a custom format string in settings dialog" + | ||
| " to improve readability of JSON logs", | ||
| timeoutMillis: 2 * DEFAULT_AUTO_DISMISS_TIMEOUT_MILLIS, |
There was a problem hiding this comment.
Can we extract a const from this, place this next to DEFAULT_AUTO_DISMISS_TIMEOUT_MILLIS, then export, and import here?
| @@ -38,7 +39,7 @@ interface PopUpMessageProps { | |||
| * @return | |||
| */ | |||
| const PopUpMessageBox = ({message}: PopUpMessageProps) => { | |||
| const {id, level, message: messageStr, title, timeoutMillis} = message; | |||
| const {id, level, button, message: messageStr, title, timeoutMillis} = message; | |||
There was a problem hiding this comment.
Shall we explain in the high-level docs:
Displays a pop-up message in an alert box with an optional action button. The pop-up can be manually dismissed or will automatically close after the specified `timeoutMillis`. If `timeoutMillis` is `0`, the pop-up will remain open until manually closed.
src/components/PopUps/index.css
Outdated
| @@ -30,6 +30,10 @@ | |||
| width: 300px; | |||
| } | |||
|
|
|||
| .pop-up-message-box-callback-button { | |||
| margin-top: 10px !important; | |||
There was a problem hiding this comment.
Instead of this, can we change
.pop-up-message-box-alert-layout {
...
display: flex;
flex-direction: column;
gap: 10px;
}
src/typings/notifications.ts
Outdated
| @@ -9,6 +17,7 @@ interface PopUpMessage { | |||
| message: string, | |||
| timeoutMillis: number, | |||
| title: string, | |||
| button?: PopUpButton, | |||
There was a problem hiding this comment.
how about
import {ButtonProps} from "@mui/joy";
- Use
ButtonPropsinstead of our ownPopUpButton. - Rename the prop as
primaryAction. (That way it makes sense for us to add a secondary action button in the future, though not strictly needed for now. )
| onClick={button.callback} | ||
| > | ||
| {button.title} | ||
| </Button> |
There was a problem hiding this comment.
Following the type change in src/typings/notifications.ts, how about
| onClick={button.callback} | |
| > | |
| {button.title} | |
| </Button> | |
| {...primaryAction}/> |
src/utils/config.ts
Outdated
| if ("" === value.timestampKey) { | ||
| result = "Timestamp key cannot be empty."; | ||
| } else if ("" === value.logLevelKey) { |
There was a problem hiding this comment.
how about checking the strings'.length against 0?
| button: { | ||
| title: "Open Settings", | ||
| callback: () => { setIsSettingsModalOpen(true); }, | ||
| }, |
There was a problem hiding this comment.
Following the type change in src/typings/notifications.ts, how about
| button: { | |
| title: "Open Settings", | |
| callback: () => { setIsSettingsModalOpen(true); }, | |
| }, | |
| primaryAction: { | |
| children: "Settings", | |
| startDecorator: <SettingsOutlinedIcon/>, | |
| onClick: () => { setIsSettingsModalOpen(true); }, | |
| }, |
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/contexts/StateContextProvider.tsx (1)
287-299: Extract timeout constant for better maintainabilityConsider extracting the timeout multiplier to a named constant for better maintainability.
+const FORMAT_STRING_POPUP_TIMEOUT_MULTIPLIER = 2; + if (getConfig(CONFIG_KEY.DECODER_OPTIONS).formatString === CONFIG_DEFAULT[CONFIG_KEY.DECODER_OPTIONS].formatString) { postPopUp({ button: { title: "Open Settings", callback: () => { setIsSettingsModalOpen(true); }, }, level: LOG_LEVEL.INFO, message: "Adding one can enhance the readability of your structured logs by customizing how fields are displayed.", - timeoutMillis: 2 * DEFAULT_AUTO_DISMISS_TIMEOUT_MILLIS, + timeoutMillis: FORMAT_STRING_POPUP_TIMEOUT_MULTIPLIER * DEFAULT_AUTO_DISMISS_TIMEOUT_MILLIS, title: "A format string has not been configured", }); }🧰 Tools
🪛 GitHub Check: lint-check
[failure] 295-295:
This line has a length of 139. Maximum allowed is 100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/PopUps/PopUpMessageBox.tsx(3 hunks)src/contexts/StateContextProvider.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/PopUps/PopUpMessageBox.tsx
🧰 Additional context used
🪛 GitHub Check: lint-check
src/contexts/StateContextProvider.tsx
[failure] 295-295:
This line has a length of 139. Maximum allowed is 100
🔇 Additional comments (4)
src/contexts/StateContextProvider.tsx (4)
64-65: LGTM: Type definitions are well-structured
The additions to StateContextType interface properly define the new state and its setter function.
Also applies to: 78-78
91-91: LGTM: Default state is properly initialized
The default state values and no-op function are correctly defined.
Also applies to: 104-104
247-248: LGTM: State hook is properly implemented
The useState hook is correctly implemented with appropriate typing and default value.
536-536: LGTM: Context value properly includes new state
The new state and setter function are correctly exposed through the context provider.
Also applies to: 549-549
| callback: () => { setIsSettingsModalOpen(true); }, | ||
| }, | ||
| level: LOG_LEVEL.INFO, | ||
| message: "Adding one can enhance the readability of your structured logs by customizing how fields are displayed.", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix line length to meet style guidelines
The message line exceeds the maximum length of 100 characters. Consider breaking it into multiple lines.
- message: "Adding one can enhance the readability of your structured logs by customizing how fields are displayed.",
+ message: "Adding one can enhance the readability of your structured logs by " +
+ "customizing how fields are displayed.",📝 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.
| message: "Adding one can enhance the readability of your structured logs by customizing how fields are displayed.", | |
| message: "Adding one can enhance the readability of your structured logs by " + | |
| "customizing how fields are displayed.", |
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 295-295:
This line has a length of 139. Maximum allowed is 100
| @@ -113,6 +114,16 @@ const PopUpMessageBox = ({message}: PopUpMessageProps) => { | |||
| <Typography level={"body-sm"}> | |||
| {messageStr} | |||
| </Typography> | |||
| {button && ( | |||
| <Button | |||
There was a problem hiding this comment.
Sorry i forgot to post this in the last batch of comments: can we right align this button?
There was a problem hiding this comment.
what do you mean specifically, can u sugggest? also the linter appears to be formatting like this. Or do u mean in UI...?
There was a problem hiding this comment.
Sorry for the confusion. I meant in the UI, we can have something like
.pop-up-message-box-actions-container {
display: flex;
justify-content: flex-end;
}
<Box className="pop-up-message-box-actions-container">
<Button/>
</Box>
Then it would look like
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
src/typings/file.ts (1)
1-4: LGTM! Consider adding JSDoc documentation.The type definition is well-structured and follows TypeScript best practices. Given its role in the new format string handling feature, consider adding JSDoc documentation to clarify its purpose and usage.
Add documentation like this:
+/** + * Callback type for file open events. + * Used to handle format string validation and popup notifications when files are opened. + * @param file The File object being opened + */ type OnFileOpenCallback = (file: File) => void; export type {OnFileOpenCallback};src/typings/worker.ts (1)
126-126: Consider adding JSDoc comment for the new response type.The type mapping is correctly defined. To maintain consistency with other complex response types in the codebase, consider adding documentation about when this response code is triggered.
Add documentation above the
WorkerRespMaptype:type WorkerRespMap = { [WORKER_RESP_CODE.CHUNK_DATA]: { logs: string }, + /** + * Triggered when the format string is empty and a file is opened. + * Used to notify the user to configure their format string. + */ [WORKER_RESP_CODE.FORMAT_POPUP]: null,src/services/MainWorker.ts (2)
53-59: Consider enhancing type safety for the format popup responseThe implementation looks good and aligns well with the PR objectives. However, we could improve type safety by explicitly defining the response type for
FORMAT_POPUP.Consider adding a type definition in the worker typings:
// In src/typings/worker.ts export interface WorkerResp<T extends WORKER_RESP_CODE> { [WORKER_RESP_CODE.FORMAT_POPUP]: null; // ... other response types }
62-63: Consider addressing the TODO comment in a future PRThe TODO comment suggests breaking up the
onmessagefunction into smaller functions. While not directly related to this PR's changes, this refactoring would improve maintainability.Would you like me to create a GitHub issue to track this technical debt?
src/contexts/StateContextProvider.tsx (3)
82-82: Consider using a more specific type for the setter functionThe setter function type could be more specific to match React's state setter pattern.
- setIsSettingsModalOpen: (isOpen: boolean) => void, + setIsSettingsModalOpen: React.Dispatch<React.SetStateAction<boolean>>,
290-291: Break long message string into multiple linesThe message string exceeds the recommended line length limit.
- message: "Adding a format string can enhance the readability of your" + - " structured logs by customizing how fields are displayed.", + message: "Adding a format string can enhance the readability of your " + + "structured logs by customizing how fields are displayed.",
288-299: Extract popup configuration to a constantConsider extracting the popup configuration to a named constant for better maintainability and reusability.
+const FORMAT_STRING_POPUP = { + level: LOG_LEVEL.INFO, + message: "Adding a format string can enhance the readability of your " + + "structured logs by customizing how fields are displayed.", + timeoutMillis: LONG_AUTO_DISMISS_TIMEOUT_MILLIS, + title: "A format string has not been configured", +}; case WORKER_RESP_CODE.FORMAT_POPUP: postPopUp({ - level: LOG_LEVEL.INFO, - message: "Adding a format string can enhance the readability of your" + - " structured logs by customizing how fields are displayed.", + ...FORMAT_STRING_POPUP, primaryAction: { children: "Settings", startDecorator: <SettingsOutlinedIcon/>, onClick: () => { setIsSettingsModalOpen(true); }, }, - timeoutMillis: LONG_AUTO_DISMISS_TIMEOUT_MILLIS, - title: "A format string has not been configured", });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
src/components/PopUps/PopUpMessageBox.tsx(3 hunks)src/components/PopUps/index.css(1 hunks)src/contexts/StateContextProvider.tsx(9 hunks)src/services/MainWorker.ts(2 hunks)src/services/decoders/ClpIrDecoder.ts(3 hunks)src/services/decoders/JsonlDecoder/index.ts(2 hunks)src/typings/file.ts(1 hunks)src/typings/formatters.ts(2 hunks)src/typings/notifications.ts(3 hunks)src/typings/worker.ts(2 hunks)src/utils/config.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/typings/formatters.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/PopUps/index.css
- src/typings/notifications.ts
- src/utils/config.ts
🧰 Additional context used
📓 Learnings (1)
src/services/decoders/ClpIrDecoder.ts (2)
Learnt from: davemarco
PR: y-scope/yscope-log-viewer#123
File: src/services/decoders/ClpIrDecoder.ts:42-42
Timestamp: 2024-11-18T01:35:35.812Z
Learning: The interface `YscopeFormatterOptionsType` is defined in `src/typings/formatters.ts` and is used to type the options passed to `YscopeFormatter`, ensuring type safety in constructor calls like `new YscopeFormatter({ formatString: decoderOptions.formatString })`.
Learnt from: davemarco
PR: y-scope/yscope-log-viewer#123
File: src/services/decoders/ClpIrDecoder.ts:90-90
Timestamp: 2024-11-18T01:35:37.012Z
Learning: Within `ClpIrDecoder.ts`, in the `setFormatterOptions` method, `options.formatString` is already typed as a string, so explicit casting to string is unnecessary.
🔇 Additional comments (9)
src/components/PopUps/PopUpMessageBox.tsx (3)
11-11: LGTM: Button import is correctly placed
The Button import is properly grouped with other MUI Joy imports.
35-37: Documentation added as requested
The high-level documentation has been added as suggested in the previous review.
44-44: LGTM: Props destructuring is well-organized
The primaryAction prop is properly destructured alongside existing properties.
src/services/decoders/ClpIrDecoder.ts (3)
16-16: LGTM! Import aligns with new popup functionality
The import of postFormatPopup supports the new feature to notify users when format string is empty.
33-33: LGTM! Clean property initialization
The direct initialization of #formatter as null with proper typing improves code clarity.
42-47: Verify consistent behaviour across decoders
Since this change affects how empty format strings are handled, we should verify that other decoders (e.g., JsonlDecoder) handle empty format strings consistently.
✅ Verification successful
Empty format string handling is consistent across decoders
The verification shows that both ClpIrDecoder and JsonlDecoder handle empty format strings consistently:
- Both use YscopeFormatter with the same format string handling
- Both trigger postFormatPopup() when format string length is 0
- Both use the same configuration defaults (empty string as default format string)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other decoder implementations and their format string handling
ast-grep --pattern 'class $DECODER implements Decoder {
$$$
constructor($$$) {
$$$
formatString
$$$
}
$$$
}'
Length of output: 130
Script:
#!/bin/bash
# Let's try a different approach to find decoder implementations
rg -l "implements Decoder"
# Then search for formatString handling in these files
rg "formatString" --type ts -A 5 -B 5
Length of output: 15401
src/typings/worker.ts (1)
80-80: LGTM! Verify usage across the codebase.
The new enum value follows the established naming convention and is properly integrated into the WORKER_RESP_CODE enum.
Let's verify its usage across the codebase:
✅ Verification successful
Implementation verified and properly integrated
The FORMAT_POPUP enum value is correctly implemented and used throughout the codebase:
- Properly dispatched in
MainWorker.tswith appropriate documentation - Correctly handled in
StateContextProvider.tsxwith a user-friendly message - Type definitions are consistent and well-integrated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of FORMAT_POPUP in the codebase
# Expected: Should find usage in worker response handling
# Search for FORMAT_POPUP usage
rg "FORMAT_POPUP" --type ts
Length of output: 313
Script:
#!/bin/bash
# Let's examine the usage context in these files to ensure proper implementation
# Check MainWorker.ts implementation
rg -B 5 -A 5 "FORMAT_POPUP" src/services/MainWorker.ts
# Check StateContextProvider.tsx handler
rg -B 5 -A 5 "FORMAT_POPUP" src/contexts/StateContextProvider.tsx
Length of output: 1010
src/services/MainWorker.ts (1)
161-161: LGTM! Export statement is properly placed and follows conventions
The named export makes the function available for use by the decoders, which aligns with the feature requirements.
src/services/decoders/JsonlDecoder/index.ts (1)
20-20: LGTM: Clean import statement addition
The import statement is properly structured and follows TypeScript conventions.
| {primaryAction && ( | ||
| <Button | ||
| color={color} | ||
| variant={"solid"} | ||
| {...primaryAction} | ||
| onClick={(event) => { | ||
| primaryAction.onClick?.(event); | ||
| handleCloseButtonClick(); | ||
| }} | ||
| > | ||
| {primaryAction.children} | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add right alignment to the primary action button
The button implementation looks good, but it's missing the right alignment requested in the previous review.
Add a container with right alignment:
{primaryAction && (
+ <Box sx={{ display: 'flex', justifyContent: 'flex-end' }}>
<Button
color={color}
variant={"solid"}
{...primaryAction}
onClick={(event) => {
primaryAction.onClick?.(event);
handleCloseButtonClick();
}}
>
{primaryAction.children}
</Button>
+ </Box>
)}📝 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.
| {primaryAction && ( | |
| <Button | |
| color={color} | |
| variant={"solid"} | |
| {...primaryAction} | |
| onClick={(event) => { | |
| primaryAction.onClick?.(event); | |
| handleCloseButtonClick(); | |
| }} | |
| > | |
| {primaryAction.children} | |
| </Button> | |
| )} | |
| {primaryAction && ( | |
| <Box sx={{ display: 'flex', justifyContent: 'flex-end' }}> | |
| <Button | |
| color={color} | |
| variant={"solid"} | |
| {...primaryAction} | |
| onClick={(event) => { | |
| primaryAction.onClick?.(event); | |
| handleCloseButtonClick(); | |
| }} | |
| > | |
| {primaryAction.children} | |
| </Button> | |
| </Box> | |
| )} |
There was a problem hiding this comment.
lol i promise this is coincidental but he rabbit suggested something similar to https://github.com/y-scope/yscope-log-viewer/pull/129/files#r1860889379
seems it didn't understand we could be adding more actions in the future though.
There was a problem hiding this comment.
@junhaoliao, thanks for the clarification. I'll keep in mind that more actions may be added in the future.
(_/)
(O_o)
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#129
File: src/components/PopUps/PopUpMessageBox.tsx:119-131
Timestamp: 2024-11-27T16:16:27.719Z
Learning: In `src/components/PopUps/PopUpMessageBox.tsx`, future enhancements may include adding multiple actions to the pop-up message box. Suggestions should account for the possibility of multiple actions rather than just a single one.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (streamType === CLP_IR_STREAM_TYPE.STRUCTURED) { | ||
| this.#formatter = new YscopeFormatter({formatString: decoderOptions.formatString}); | ||
| if (0 === decoderOptions.formatString.length) { | ||
| postFormatPopup(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add validation and optimize formatter creation
The current implementation has potential issues:
- No validation of
decoderOptionsorformatString - Creates formatter even when format string is empty
- Potential undefined access on
formatString.length
Consider this safer implementation:
if (streamType === CLP_IR_STREAM_TYPE.STRUCTURED) {
+ if (!decoderOptions?.formatString) {
+ postFormatPopup();
+ // Set empty string as default
+ decoderOptions = { ...decoderOptions, formatString: "" };
+ } else if (decoderOptions.formatString.length === 0) {
+ postFormatPopup();
+ }
this.#formatter = new YscopeFormatter({formatString: decoderOptions.formatString});
- if (0 === decoderOptions.formatString.length) {
- postFormatPopup();
- }
}📝 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.
| if (streamType === CLP_IR_STREAM_TYPE.STRUCTURED) { | |
| this.#formatter = new YscopeFormatter({formatString: decoderOptions.formatString}); | |
| if (0 === decoderOptions.formatString.length) { | |
| postFormatPopup(); | |
| } | |
| } | |
| if (streamType === CLP_IR_STREAM_TYPE.STRUCTURED) { | |
| if (!decoderOptions?.formatString) { | |
| postFormatPopup(); | |
| // Set empty string as default | |
| decoderOptions = { ...decoderOptions, formatString: "" }; | |
| } else if (decoderOptions.formatString.length === 0) { | |
| postFormatPopup(); | |
| } | |
| this.#formatter = new YscopeFormatter({formatString: decoderOptions.formatString}); | |
| } |
| if (0 === decoderOptions.formatString.length) { | ||
| postFormatPopup(); | ||
| } |
There was a problem hiding this comment.
Add null/undefined check for formatString
While the empty string check is good, we should also handle cases where formatString might be undefined.
Consider this safer implementation:
- if (0 === decoderOptions.formatString.length) {
+ if (!decoderOptions.formatString?.length) {
postFormatPopup();
}📝 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.
| if (0 === decoderOptions.formatString.length) { | |
| postFormatPopup(); | |
| } | |
| if (!decoderOptions.formatString?.length) { | |
| postFormatPopup(); | |
| } |
💡 Codebase verification
The review comment's concern is valid but for a different reason
The popup is triggered in a Web Worker context where JsonlDecoder is instantiated only once per file through LogFileManager. However, the current implementation still has two decoders (JsonlDecoder and ClpIrDecoder) that can trigger the same popup, which could lead to duplicate notifications.
- Move the format popup logic to
LogFileManagerwhere it can be triggered once during file initialization, regardless of the decoder type. - Consider using a session flag in the worker context to ensure the popup appears only once per session.
🔗 Analysis chain
Consider moving popup trigger to a more appropriate location
Triggering the popup in the constructor might lead to multiple popups if multiple decoder instances are created. Consider:
- Moving this logic to a higher-level component
- Adding a flag to ensure the popup is shown only once per session
Let's verify if multiple decoders might be instantiated:
Let me gather more context about where and how the decoder is used.
Let me check one more thing to understand the worker communication pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for JsonlDecoder instantiation patterns
ast-grep --pattern 'new JsonlDecoder($$$)'
Length of output: 151
Script:
#!/bin/bash
# Check how LogFileManager is instantiated
ast-grep --pattern 'new LogFileManager($$$)'
# Check the implementation context of postFormatPopup
rg "postFormatPopup" -A 5
Length of output: 2531
Script:
#!/bin/bash
# Check for WORKER_RESP_CODE usage and worker communication
rg "WORKER_RESP_CODE" -A 3
# Check where the format popup is handled
rg "FORMAT_POPUP" -B 2 -A 2
Length of output: 9059
|
I made requested changes. I also modified logic that triggers popup. Previously, the popup was triggered by front-end when file was opened. As you mentioned, not ideal since not all files are structured. It is not trivial to just check extension since the same extension for IRV1 and IRV2... Instead, I added functionality for the backend worker to trigger popup in front end. Now can trigger popup specifically when formatter in constructed in decoder constructor (i.e. triggered only for structured decoders). I also added a feature that closes the popup when you click the button. |
| onClick={(event) => { | ||
| primaryAction.onClick?.(event); | ||
| handleCloseButtonClick(); | ||
| }} |
There was a problem hiding this comment.
Let's extract a handler from this:
import React, {
(NOTE: below we name the event ev for consistency. )
const handlePrimaryActionClick = (ev: React.MouseEvent<HTMLButtonElement>) => {
primaryAction?.onClick?.(ev);
handleCloseButtonClick();
};
| onClick={(event) => { | |
| primaryAction.onClick?.(event); | |
| handleCloseButtonClick(); | |
| }} | |
| onClick={handlePrimaryActionClick} |
| {primaryAction && ( | ||
| <Button | ||
| color={color} | ||
| variant={"solid"} | ||
| {...primaryAction} | ||
| onClick={(event) => { | ||
| primaryAction.onClick?.(event); | ||
| handleCloseButtonClick(); | ||
| }} | ||
| > | ||
| {primaryAction.children} | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
lol i promise this is coincidental but he rabbit suggested something similar to https://github.com/y-scope/yscope-log-viewer/pull/129/files#r1860889379
seems it didn't understand we could be adding more actions in the future though.
src/components/PopUps/index.css
Outdated
| @@ -27,6 +27,9 @@ | |||
| } | |||
|
|
|||
| .pop-up-message-box-alert-layout { | |||
| display: flex; | |||
| flex-direction: column; | |||
| gap: 10px; | |||
| width: 300px; | |||
There was a problem hiding this comment.
How about
| width: 300px; | |
| width: 333px; |
| \`{@timestamp:timestamp:YYYY-MM-DD HH\\:mm\\:ss.SSS}\`. The default setting is | ||
| an empty string which will print all fields formatted as JSON.`, |
There was a problem hiding this comment.
How about
| \`{@timestamp:timestamp:YYYY-MM-DD HH\\:mm\\:ss.SSS}\`. The default setting is | |
| an empty string which will print all fields formatted as JSON.`, | |
| \`{@timestamp:timestamp:YYYY-MM-DD HH\\:mm\\:ss.SSS}\`. Leave this blank to display the | |
| entire log event formatted as JSON.`, |
| @@ -70,6 +79,7 @@ interface StateContextType { | |||
| exportLogs: () => void, | |||
| loadFile: (fileSrc: FileSrcType, cursor: CursorType) => void, | |||
| loadPageByAction: (navAction: NavigationAction) => void, | |||
| setIsSettingsModalOpen: (isOpen: boolean) => void, | |||
There was a problem hiding this comment.
For consistency, how about
| setIsSettingsModalOpen: (isOpen: boolean) => void, | |
| setIsSettingsModalOpen: (newIsOpen: boolean) => void, |
There was a problem hiding this comment.
I prefer without new. I think the new for log level filter was just there to solve a namespace issue (i think loglevelfilter taken). Instead, i left as isOpen, and changed newLogLevelFilter to just filter.
| * Sends a message to the renderer to open a pop-up which prompts user to replace the default | ||
| * format string. | ||
| */ | ||
| const postFormatPopup = () => { |
There was a problem hiding this comment.
For now this is fine. In a future PR, we can refactor notification & exception handling in the service worker so that every notification at any level can only be sent by either a notification dispatcher (like this postFormatPopup) or the exception catching ~L147. What do you think?
There was a problem hiding this comment.
I think we can do something like this. I think we will need to add codes to the notifications so front-end knows if its an error or generic notifcation. I originally planned to do something like this and also just send the entire popup object to the front end from the back end. However, adding the settings modal callback in the backend is a bit gross, so decided against it. In future i think tho, we can have a unified notification handler in front-end, and maybe add codes (one could be for constructing format string popup with settings modal, another for error, etc..) Should we add issue?
There was a problem hiding this comment.
Sure. Let's add the issue.
There was a problem hiding this comment.
@coderabbitai can you add github issue to unify popup handler and notification handler in the front-end per my comment?
There was a problem hiding this comment.
@davemarco, I've created the GitHub issue to unify the popup handler and notification handler in the front-end as per your request: Issue #131.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue as requested by @davemarco
gh issue create \
--title "Unify popup handler and notification handler in the front-end" \
--body "As discussed in [PR #129](https://github.com/y-scope/yscope-log-viewer/pull/129) and [this comment](https://github.com/y-scope/yscope-log-viewer/pull/129#discussion_r1860925043), we should unify the popup handler and notification handler in the front-end.
Requester: @davemarco." \
--assignee davemarco
Length of output: 455
davemarco
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/PopUps/index.css (1)
36-38: Consider adding gap property for better button spacing.While the flex layout with end justification works well, consider adding a gap property to ensure proper spacing between multiple action buttons if they're added in the future. Also, for accessibility, consider adding appropriate focus styles.
.pop-up-message-box-actions-container { display: flex; justify-content: flex-end; + gap: 8px; + /* Ensure buttons are clearly visible when focused */ + :focus-visible { + outline: 2px solid var(--joy-palette-primary-500); + outline-offset: 2px; + } }src/components/PopUps/PopUpMessageBox.tsx (2)
35-37: Enhance JSDoc documentation with primaryAction detailsThe JSDoc comments are clear but could be more comprehensive. Consider adding details about the
primaryActionparameter and its usage./** * Displays a pop-up message in an alert box with an optional action button. The pop-up can * be manually dismissed or will automatically close after the specified `timeoutMillis`. * If `timeoutMillis` is `0`, the pop-up will remain open until manually closed. + * + * @param props.message.primaryAction - Optional action button configuration including onClick handler and children */
124-135: Consider restructuring for future multiple actionsWhile the current implementation works well for a single action, consider restructuring to accommodate multiple actions in the future:
- {primaryAction && ( - <Box className={"pop-up-message-box-actions-container"}> - <Button - color={color} - variant={"solid"} - {...primaryAction} - onClick={handlePrimaryActionClick} - > - {primaryAction.children} - </Button> - </Box> - )} + <Box className={"pop-up-message-box-actions-container"}> + {primaryAction && ( + <Button + color={color} + variant={"solid"} + {...primaryAction} + onClick={handlePrimaryActionClick} + > + {primaryAction.children} + </Button> + )} + {/* Future secondary actions can be added here */} + </Box>This structure will make it easier to add secondary actions in the future while maintaining consistent spacing and alignment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/components/PopUps/PopUpMessageBox.tsx(5 hunks)src/components/PopUps/index.css(1 hunks)src/components/modals/SettingsModal/SettingsDialog.tsx(1 hunks)src/contexts/StateContextProvider.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/modals/SettingsModal/SettingsDialog.tsx
🧰 Additional context used
📓 Learnings (1)
src/components/PopUps/PopUpMessageBox.tsx (1)
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#129
File: src/components/PopUps/PopUpMessageBox.tsx:119-131
Timestamp: 2024-11-27T16:16:28.173Z
Learning: In `src/components/PopUps/PopUpMessageBox.tsx`, future enhancements may include adding multiple actions to the pop-up message box. Suggestions should account for the possibility of multiple actions rather than just a single one.
🔇 Additional comments (8)
src/components/PopUps/index.css (1)
30-34: LGTM! Changes align with previous review feedback.
The implementation of flex layout with proper spacing and the adjusted width improve the pop-up's visual structure.
src/components/PopUps/PopUpMessageBox.tsx (2)
11-11: LGTM: Clean interface updates
The addition of Button import and primaryAction destructuring is well-structured and aligns with the component's enhanced functionality.
Also applies to: 44-44
54-57: LGTM: Well-structured event handler
The extracted handler function is clean, properly typed, and effectively chains the primary action with popup closure.
src/contexts/StateContextProvider.tsx (5)
11-26: LGTM! Import statements are well-organized.
The new imports are properly organized and necessary for the added functionality.
Line range hint 68-83: LGTM! Type definitions are consistent and well-structured.
The state management for settings modal follows React's useState pattern and maintains consistency with existing code structure.
Also applies to: 95-108
251-252: LGTM! State management is properly implemented.
The settings modal state is correctly initialized using useState hook, maintaining consistency with other state declarations.
449-456: LGTM! Log level filter implementation is solid.
The setLogLevelFilter callback is well-implemented with proper worker communication and cursor handling.
Line range hint 541-554: LGTM! Context value updates are consistent.
The settings modal state and setter are properly exposed through the context provider.
| case WORKER_RESP_CODE.FORMAT_POPUP: | ||
| postPopUp({ | ||
| level: LOG_LEVEL.INFO, | ||
| message: "Adding a format string can enhance the readability of your" + | ||
| " structured logs by customizing how fields are displayed.", | ||
| primaryAction: { | ||
| children: "Settings", | ||
| startDecorator: <SettingsOutlinedIcon/>, | ||
| onClick: () => { setIsSettingsModalOpen(true); }, | ||
| }, | ||
| timeoutMillis: LONG_AUTO_DISMISS_TIMEOUT_MILLIS, | ||
| title: "A format string has not been configured", | ||
| }); | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix line length and improve message text.
The message text exceeds the line length limit and could be more concise.
Apply this diff to improve readability:
- message: "Adding a format string can enhance the readability of your" +
- " structured logs by customizing how fields are displayed.",
+ message: "Add a format string to customize how log fields are displayed.",📝 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.
| case WORKER_RESP_CODE.FORMAT_POPUP: | |
| postPopUp({ | |
| level: LOG_LEVEL.INFO, | |
| message: "Adding a format string can enhance the readability of your" + | |
| " structured logs by customizing how fields are displayed.", | |
| primaryAction: { | |
| children: "Settings", | |
| startDecorator: <SettingsOutlinedIcon/>, | |
| onClick: () => { setIsSettingsModalOpen(true); }, | |
| }, | |
| timeoutMillis: LONG_AUTO_DISMISS_TIMEOUT_MILLIS, | |
| title: "A format string has not been configured", | |
| }); | |
| break; | |
| case WORKER_RESP_CODE.FORMAT_POPUP: | |
| postPopUp({ | |
| level: LOG_LEVEL.INFO, | |
| message: "Add a format string to customize how log fields are displayed.", | |
| primaryAction: { | |
| children: "Settings", | |
| startDecorator: <SettingsOutlinedIcon/>, | |
| onClick: () => { setIsSettingsModalOpen(true); }, | |
| }, | |
| timeoutMillis: LONG_AUTO_DISMISS_TIMEOUT_MILLIS, | |
| title: "A format string has not been configured", | |
| }); | |
| break; |
Description
PR does three things
Adds special case to formatter for empty for string
PR modifies
YScopeFormatterso that is will print all the log event fields as JSON when the format string is empty. This allows users to see their logs, so they know what fields to format. PR also sets the default format string to empty.Popup to remind users to set format string
PR adds a popup to notify users to set format string. The popup only appears when the format string is empty, i.e. the default. It will not appear if they change the format string. The popup opens when users open the file, not when the log viewer starts. I thought this was more appropriate because again they can only set the format string once they see their logs (i.e. after the file is open).
Support button on popup
I modified the popup and settings modal, so the popup can have a button to actually open the settings.
Validation performed
Tested empty format string, popup and button
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes