Skip to content
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

feat(URL): Share link with search parameters #173

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

Henry8192
Copy link
Collaborator

@Henry8192 Henry8192 commented Jan 27, 2025

Description

Add support to making queries in the url. Also, a button that generates link to current query is added in SearchTabPanel.

Validation performed

Summary by CodeRabbit

Release Notes

  • New Features

    • Added a share button in the search tab panel to copy a permalink with current search parameters.
    • Enhanced URL context to support case sensitivity, regex, and query string parameters.
  • Improvements

    • Improved state management for file loading and query handling.
    • Added dynamic query triggering based on URL parameters.
    • Enhanced date parsing capabilities in the log viewer.
  • Technical Updates

    • Expanded URL search parameters to provide more detailed search configurations.
    • Updated query handling to utilize new parameters for improved functionality.
    • Introduced a method for formatting unstructured log events.

This update introduces more flexible search functionality and enhances the user experience for sharing search settings.

Copy link

coderabbitai bot commented Jan 27, 2025

Walkthrough

The pull request enhances the SearchTabPanel component and related context providers by integrating new functionalities for managing search parameters, including case sensitivity, regex options, and query string parameters. A new sharing feature is introduced, allowing users to copy a permalink with the current search parameters. Key modifications include updates to the UrlContextProvider, StateContextProvider, and SearchTabPanel, improving the overall state management and user experience when handling search configurations.

Changes

File Change Summary
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx Introduced handleShareButtonClick function; updated state initialization for queryString, queryIsCaseSensitive, and queryIsRegex; modified rendering to include a share button.
src/contexts/StateContextProvider.tsx Updated startQuery function signature; removed redundant fileName declaration; reset queryResults before starting a new query.
src/contexts/UrlContextProvider.tsx Added new properties to URL_HASH_PARAMS_DEFAULT; modified functions to handle new hash parameters for case sensitivity and regex.
src/typings/url.ts Updated HASH_PARAM_NAMES enum to include QUERY_IS_CASE_SENSITIVE, QUERY_IS_REGEX, and QUERY_STRING; modified UrlHashParams interface to reflect new parameters.
src/services/LogFileManager/index.ts Renamed parameters in startQuery method from isRegex and isCaseSensitive to queryIsRegex and queryIsCaseSensitive.
src/typings/query.ts Renamed properties in QueryArgs interface from isCaseSensitive to queryIsCaseSensitive and from isRegex to queryIsRegex.
src/typings/worker.ts Updated WorkerReqMap to replace isRegex and isCaseSensitive with queryIsRegex and queryIsCaseSensitive.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx Refactored QueryInputBox to accept props for query parameters instead of managing internal state; updated event handling logic accordingly.

Possibly Related PRs

Suggested Reviewers

  • junhaoliao

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 82b39d1 and 7f240e6.

📒 Files selected for processing (4)
  • src/components/Editor/MonacoInstance/language.ts (1 hunks)
  • src/services/decoders/ClpIrDecoder/index.ts (3 hunks)
  • src/components/Editor/MonacoInstance/language.ts (1 hunks)
  • src/services/decoders/ClpIrDecoder/index.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...

**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Prefer false == <expression> rather than !<expression>.

  • src/components/Editor/MonacoInstance/language.ts
  • src/services/decoders/ClpIrDecoder/index.ts
🧠 Learnings (1)
src/services/decoders/ClpIrDecoder/index.ts (1)
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 })`.
🔇 Additional comments (11)
src/components/Editor/MonacoInstance/language.ts (1)

36-36: Improved date regex pattern - good enhancement!

The new regex pattern is more flexible and can match a wider variety of date-time formats commonly found in logs, including:

  • Basic dates (YYYY-MM-DD)
  • Dates with time using space or 'T' separator
  • Optional fractional seconds with dot or comma
  • Optional timezone information (Z or +/-HH:MM)

This improves the tokenizer's ability to correctly highlight date strings in different formats.

src/services/decoders/ClpIrDecoder/index.ts (10)

1-4: No issues observed with these import statements.
They appear consistent and correctly scoped.


7-7: No concerns with the new import for Nullable.
This import is straightforward and aligns with prior usage in this repository.


14-19: All newly introduced imports appear valid and consistent.
They reference existing modules and do not exhibit any syntactical or logical issues.


23-23: Import from JsonlDecoder/utils looks good.
No problematic references found here.


24-24: parseFilterKeys import is valid and helpful.
No conflicts or potential breakage detected.


25-29: Structure of imported utilities from ./utils is appropriate.
These references are necessary for IR stream handling and appear well-organised.


37-38: Private property #structuredIrNamespaceKeys is clearly named and typed.
No functional or naming conflicts detected.


42-43: Constructor signature update is appropriate.
Accepting a MainModule and a Uint8Array is consistent with typical usage in this codebase.


54-58: Conditional creation of a formatter for structured streams is clear and correct.
The code handles empty formatString by calling postFormatPopup, which is a sensible user alert.


79-79: Factory method usage is well-structured.
Returning a new instance of ClpIrDecoder is a clean approach, with no concurrency or resource issues evident.

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)

80-86: Consider omitting the empty object parameter.

The empty object passed as hashParamsUpdates is unnecessary.

 const handleShareButtonClick = () => {
     copyPermalinkToClipboard({
         queryString: queryString,
         isCaseSensitive: getIsCaseSensitive(queryOptions),
         isRegex: getIsRegex(queryOptions),
-    }, {});
+    });
 };
src/contexts/StateContextProvider.tsx (1)

459-468: Simplify the search parameter condition.

The condition is complex and could be simplified. Also, consider moving the URL parameter reset to a cleanup function.

 useEffect(() => {
-    if (URL_SEARCH_PARAMS_DEFAULT.queryString !== queryString && URL_SEARCH_PARAMS_DEFAULT.isCaseSensitive !== isCaseSensitive && URL_SEARCH_PARAMS_DEFAULT.isRegex !== isRegex) {
+    const hasSearchParams = queryString !== URL_SEARCH_PARAMS_DEFAULT.queryString ||
+        isCaseSensitive !== URL_SEARCH_PARAMS_DEFAULT.isCaseSensitive ||
+        isRegex !== URL_SEARCH_PARAMS_DEFAULT.isRegex;
+    
+    if (hasSearchParams) {
         startQuery({queryString, isCaseSensitive, isRegex});
     }
-    updateWindowUrlSearchParams({
-        [SEARCH_PARAM_NAMES.QUERY_STRING]: URL_SEARCH_PARAMS_DEFAULT.queryString,
-        [SEARCH_PARAM_NAMES.IS_CASE_SENSITIVE]: URL_SEARCH_PARAMS_DEFAULT.isCaseSensitive,
-        [SEARCH_PARAM_NAMES.IS_REGEX]: URL_SEARCH_PARAMS_DEFAULT.isRegex,
-    });
+    
+    return () => {
+        updateWindowUrlSearchParams({
+            [SEARCH_PARAM_NAMES.QUERY_STRING]: URL_SEARCH_PARAMS_DEFAULT.queryString,
+            [SEARCH_PARAM_NAMES.IS_CASE_SENSITIVE]: URL_SEARCH_PARAMS_DEFAULT.isCaseSensitive,
+            [SEARCH_PARAM_NAMES.IS_REGEX]: URL_SEARCH_PARAMS_DEFAULT.isRegex,
+        });
+    };
 }, [isFileLoaded]);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 393f8f8 and d398cee.

📒 Files selected for processing (4)
  • src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (4 hunks)
  • src/contexts/StateContextProvider.tsx (4 hunks)
  • src/contexts/UrlContextProvider.tsx (2 hunks)
  • src/typings/url.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/contexts/UrlContextProvider.tsx (1)

Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.

src/typings/url.ts (1)

Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.

src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)

Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.

src/contexts/StateContextProvider.tsx (1)

Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.

🔇 Additional comments (9)
src/typings/url.ts (2)

6-8: LGTM! The new search parameter names are well-defined.

The added enum values follow consistent naming conventions and use appropriate camelCase for URL parameters.


17-19: LGTM! The interface properties are well-typed.

The added properties use appropriate types and maintain type safety through computed property names.

src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (3)

16-16: LGTM! The imports support the new sharing functionality.

The added imports are necessary and well-organized.

Also applies to: 21-24


72-72: LGTM! The URL context integration is well-implemented.

The context usage and state initialization follow React best practices with proper null handling.

Also applies to: 75-75


131-136: LGTM! The share button UI is well-integrated.

The button follows consistent patterns and provides clear user feedback through the tooltip.

src/contexts/UrlContextProvider.tsx (1)

27-29: LGTM! The default parameters are consistently defined.

The new search parameters follow the established pattern and ordering.

src/contexts/StateContextProvider.tsx (3)

249-249: LGTM! The URL context usage is complete.

All necessary search parameters are extracted in a consistent order.


254-255: LGTM! The file loading state is well-managed.

The state is properly initialized and updated in the file info handler.

Also applies to: 309-309


424-425: LGTM! The query state reset is appropriate.

Query results and progress are properly reset when loading a new file.

@junhaoliao
Copy link
Member

(no rush; whenever you have time; preferably after your vacation)
hey @Henry8192 , if the PR is ready for review, can you resolve all conflicts and fill out the PR description? then please request a review from me and we can discuss accordingly.

Copy link
Member

@junhaoliao junhaoliao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great job. the changes are mostly clean but i think we can make some structural improvements.
please do merge yscope/main into your branch before making further changes, to avoid more conflicts.

@Henry8192 Henry8192 marked this pull request as draft January 29, 2025 13:11
@Henry8192 Henry8192 marked this pull request as ready for review February 27, 2025 20:31
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (3)

53-53: Consider restructuring to avoid disabling the max-lines-per-function lint rule.
If feasible, reduce the function’s size by extracting logic into smaller helper functions, which can improve maintainability and readability.


118-119: Adhere to the coding guideline for negation checks.
Replace !queryIsCaseSensitive with false == queryIsCaseSensitive for consistency.

-handleQuerySubmit({queryIsCaseSensitive: !queryIsCaseSensitive});
-setQueryIsCaseSensitive(!queryIsCaseSensitive);
+handleQuerySubmit({queryIsCaseSensitive: false == queryIsCaseSensitive});
+setQueryIsCaseSensitive(false == queryIsCaseSensitive);

123-124: Maintain consistency for the regex negation as well.
Similarly, use false == queryIsRegex instead of !queryIsRegex.

-handleQuerySubmit({queryIsRegex: !queryIsRegex});
-setQueryIsRegex(!queryIsRegex);
+handleQuerySubmit({queryIsRegex: false == queryIsRegex});
+setQueryIsRegex(false == queryIsRegex);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d398cee and c6b97a9.

📒 Files selected for processing (7)
  • src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (8 hunks)
  • src/contexts/StateContextProvider.tsx (4 hunks)
  • src/contexts/UrlContextProvider.tsx (4 hunks)
  • src/services/LogFileManager/index.ts (2 hunks)
  • src/typings/query.ts (1 hunks)
  • src/typings/url.ts (1 hunks)
  • src/typings/worker.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/contexts/StateContextProvider.tsx
  • src/typings/url.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...

**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Prefer false == <expression> rather than !<expression>.

  • src/typings/worker.ts
  • src/typings/query.ts
  • src/services/LogFileManager/index.ts
  • src/contexts/UrlContextProvider.tsx
  • src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
🪛 GitHub Check: lint-check
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx

[failure] 81-81:
This line has a length of 109. Maximum allowed is 100


[failure] 86-86:
React Hook useEffect has missing dependencies: 'startQuery', 'urlQueryIsCaseSensitive', and 'urlQueryIsRegex'. Either include them or remove the dependency array

🪛 GitHub Actions: lint
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx

[warning] 81-81: This line has a length of 109. Maximum allowed is 100 @stylistic/max-len

🔇 Additional comments (22)
src/typings/worker.ts (1)

115-116: LGTM! Parameter renaming improves code clarity.

The renaming of parameters to queryIsCaseSensitive and queryIsRegex enhances code readability by explicitly indicating these parameters are related to query operations.

src/typings/query.ts (1)

3-4: LGTM! Consistent parameter naming improves maintainability.

Renaming these properties to queryIsCaseSensitive and queryIsRegex aligns with the changes in worker.ts, ensuring consistency across the codebase.

src/services/LogFileManager/index.ts (3)

295-296: LGTM! JSDoc updates match parameter renaming.

The JSDoc parameter descriptions have been correctly updated to reflect the renamed parameters.


298-298: Function signature properly updated to match type changes.

The method signature has been updated to use the new parameter names, maintaining consistency with the type changes.


312-317: LGTM! Variable references updated to match new parameter names.

The internal code has been appropriately updated to reference the new parameter names.

src/contexts/UrlContextProvider.tsx (4)

34-36: LGTM! New default hash parameters added for query sharing.

The addition of these parameters to the default object enables the sharing feature for search parameters.


107-107: LGTM! Condition properly handles false boolean values.

The condition now correctly removes parameters when their value is false, which aligns with the expected behavior for boolean toggle parameters.


187-190: Boolean type conversion is required for URL parameters.

The UrlSearchParams interface may define boolean types, but URLSearchParams stores them as strings. Type conversion is necessary for type safety.


221-235: LGTM! Proper handling of query parameters from URL hash.

The implementation correctly retrieves and type-converts the query parameters from the URL hash. The string to boolean conversion is properly handled using string comparison.

src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (13)

3-3: No concerns regarding the import of useEffect.
It is properly added and necessary for the upcoming effect logic.


15-15: The new icon import is a beneficial addition.
Importing ShareIcon is straightforward and aligns with the sharing feature.


20-25: Appropriate imports from UrlContextProvider.
They are correctly declared and will be useful for URL manipulation and sharing.


30-33: UI_ELEMENT and UI_STATE usage is correct.
These are likely being used in the effect and logic with no apparent issues.


56-60: Destructuring queryString, queryIsCaseSensitive, queryIsRegex is implemented correctly.
No concerns here; it cleanly retrieves values from the UrlContext.


63-64: State initialization for case sensitivity and regex flags is clear.
Setting both to false by default is logical.


94-101: handleShareButtonClick function is straightforward.
Copying the permalink to the clipboard appears well-implemented for sharing the link.


105-106: handleQuerySubmit integration looks correct.
Referencing the new case-sensitivity and regex state flags aligns with the updated logic.


130-130: No concerns with the updated panel container line.
The panel’s setup remains intact.


134-151: New share button in the panel UI is well integrated.
It complements the existing collapse/expand button, enhancing user experience for link sharing.


161-161: Binding the textarea’s value to queryString is correct.
Ensures the component reflects the current search input accurately.


170-171: isChecked={queryIsCaseSensitive} logic is fine.
No issues flagged by the new approach for toggling case-sensitivity.


182-183: isChecked={queryIsRegex} logic is also good.
The toggle correctly binds to the updated state for regex mode.

@Henry8192 Henry8192 requested a review from a team as a code owner February 27, 2025 22:15
@Henry8192 Henry8192 requested a review from junhaoliao February 27, 2025 22:16
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)

78-103: Follow coding guidelines for conditional checks.

According to the coding guidelines, false == <expression> should be preferred over !<expression>.

-            if (null !== urlQueryString) {
+            if (null !== urlQueryString) {

While this particular line already follows the guidelines, ensure all conditional checks throughout the component maintain this style.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c6b97a9 and 7dd50b9.

📒 Files selected for processing (1)
  • src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...

**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Prefer false == <expression> rather than !<expression>.

  • src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
🔇 Additional comments (5)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (5)

108-115: Good implementation of sharing functionality.

The handleShareButtonClick method correctly captures the current search state and uses copyPermalinkToClipboard to share it.


70-76: Good use of refs to track URL context values.

Using refs to track the URL context values helps prevent unnecessary re-renders while still keeping the component responsive to URL changes.


91-96: Good formatting for line length compliance.

The multi-line formatting for the hash parameters object effectively prevents line length issues that were flagged in previous reviews.


148-165: Well-structured UI addition for share button.

The share button is implemented consistently with the existing UI patterns, using the same component structure as the collapse/expand button.


99-103: Properly handled useEffect dependencies.

The useEffect now correctly includes startQuery in the dependency array, addressing the issue raised in the previous review.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)

103-107: Consider adding URL query parameters to the dependency array.

The useEffect dependency array is missing urlQueryIsCaseSensitive and urlQueryIsRegex. Even though you're using refs to track these values, changes to these URL parameters should trigger the effect to ensure the component properly reacts to URL changes.

 }, [
     startQuery,
     uiState,
     urlQueryString,
+    urlQueryIsCaseSensitive,
+    urlQueryIsRegex,
 ]);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7dd50b9 and 691a911.

📒 Files selected for processing (2)
  • src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (8 hunks)
  • src/services/LogFileManager/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/services/LogFileManager/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...

**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Prefer false == <expression> rather than !<expression>.

  • src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
🔇 Additional comments (6)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (6)

112-119: Good implementation of share functionality.

The handleShareButtonClick function correctly implements the link sharing feature by calling copyPermalinkToClipboard with the current search parameters.


136-137: Toggle implementation using negation operator.

According to the coding guidelines, false == <expression> should be preferred over !<expression>. However, as discussed in previous comments, using ! here makes the toggling action more intuitive to understand.


152-169: Well-structured UI for sharing functionality.

The addition of the share button in the title area is cleanly implemented with proper tooltips and icon. Good job maintaining consistent UI patterns with the existing collapse/expand button.


95-101: Consider the intent of resetting URL parameters.

This code updates the window URL hash parameters to their default values right after starting a query with custom parameters. Is the intention to reset the URL after starting the query? If not, this might counteract the sharing functionality you're implementing.

Please confirm if this is intentional behavior - typically when implementing shareable URLs, you would want the URL to reflect the current search parameters rather than resetting them to defaults.


84-93: Good state initialization from URL parameters.

The implementation correctly initializes the search component's state from URL parameters and starts the query with those parameters when the UI is ready.


67-76: Effective use of refs for tracking URL context values.

Using refs to track the URL context values between renders is a good approach to maintain values while avoiding unnecessary re-renders.

# Conflicts:
#	src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx (1)

21-30: Interface definition follows good practices but contains lint suppressions

The new interface queryStates provides good structure for the component props. However, the ESLint suppressions for the boolean prop naming convention suggest potential inconsistency with project standards.

Consider renaming the boolean props to follow the React convention (with is or has prefix) instead of disabling the linting rule. This would better align with React best practices and eliminate the need for lint suppressions.

 interface queryStates {
     queryString: string;
-    // eslint-disable-next-line react/boolean-prop-naming
-    queryIsCaseSensitive: boolean;
-    // eslint-disable-next-line react/boolean-prop-naming
-    queryIsRegex: boolean;
+    isQueryCaseSensitive: boolean;
+    isQueryRegex: boolean;
     setQueryString: (queryString: string) => void;
-    setQueryIsCaseSensitive: (queryIsCaseSensitive: boolean) => void;
-    setQueryIsRegex: (queryIsRegex: boolean) => void;
+    setIsQueryCaseSensitive: (isQueryCaseSensitive: boolean) => void;
+    setIsQueryRegex: (isQueryRegex: boolean) => void;
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 691a911 and 82b39d1.

📒 Files selected for processing (2)
  • src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx (5 hunks)
  • src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...

**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Prefer false == <expression> rather than !<expression>.

  • src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx
  • src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
🧠 Learnings (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#107
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx:34-58
Timestamp: 2024-11-10T16:46:53.300Z
Learning: When suggesting performance optimizations, ensure they are significant and applicable to the specific context of the code being reviewed.
🔇 Additional comments (17)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx (6)

44-51: Refactoring to prop-driven model enhances component reusability

Converting from internal state management to a prop-driven model is a good design decision. This improves component reusability and allows the parent component to control the state.


57-59: Proper use of query parameters in startQuery function

The updated code correctly uses the props for query parameters, which maintains consistency with the new component structure.


70-72: State update follows established pattern but could follow coding guidelines

The code correctly toggles the case sensitivity state and updates the query, maintaining a consistent pattern throughout the component.

Note that a previous review suggested using false == queryIsCaseSensitive instead of !queryIsCaseSensitive to follow coding guidelines, but this was discussed offline and the current approach was preferred for clarity.


75-77: Consistent state update for regex toggle

The implementation follows the same pattern as the case sensitivity toggle, providing a consistent approach to state management.


96-97: Property binding correctly reflects current state

The isChecked property now properly reflects the current state from props rather than internal state, which is consistent with the component's new structure.


108-109: Property binding correctly reflects current state

The isChecked property now properly reflects the current state from props rather than internal state, maintaining consistency with the component's new structure.

src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (11)

3-5: Additional hooks imported to support new functionality

The added imports (useEffect and useRef) are necessary for implementing the new state management and synchronization features.


13-13: Import for new share functionality

The ShareIcon import supports the new URL sharing feature, which aligns with the PR objectives.


18-24: Appropriate imports for URL context functionality

The imports from UrlContextProvider provide necessary functions and constants for handling URL parameters, which is essential for the new sharing feature.


44-49: Enhanced context usage for state management

The component now retrieves additional values from both contexts to properly manage state and synchronize with URL parameters.


52-54: Added state variables for search parameters

The state variables are properly initialized and will be used to control the search parameters, allowing for synchronization with URL parameters.


56-57: Added refs to track state across renders

The refs are used to track the case sensitivity and regex settings across renders, which helps with synchronization issues. This is a valid approach, though there might be more direct alternatives.


62-71: Implemented URL sharing functionality

The handleShareButtonClick function correctly implements the feature to share the current search parameters via URL, meeting the PR objectives. The empty string check for queryString is a good optimization.


81-110: Resolve missing dependencies in useEffect hook

The useEffect hook that handles the UI state changes is missing dependencies in its dependency array. While startQuery is now included, urlQueryIsCaseSensitive and urlQueryIsRegex are still missing.

Although you're using refs as a workaround, it's generally better to include all dependencies directly in the dependency array for clarity and to follow React's best practices.

 useEffect(() => {
     if (uiState === UI_STATE.FILE_LOADING) {
         // ...
     } else if (uiState === UI_STATE.READY) {
         // ...
     }
 }, [
     startQuery,
     uiState,
     urlQueryString,
+    urlQueryIsCaseSensitive,
+    urlQueryIsRegex,
 ]);
#!/bin/bash
# Check for other similar patterns in the codebase where refs might be used instead of direct dependencies

echo "Searching for useEffect implementations with refs:"
rg "useEffect.*\{" -A 10 -B 2 | rg -B 10 "\.current" | rg -A 5 "\], \["

99-103: Address line length linting issue

Line 101 exceeds the maximum line length of 100 characters. This was flagged in a previous review and should be fixed by breaking the line into multiple lines.

 updateWindowUrlHashParams({
     [HASH_PARAM_NAMES.QUERY_STRING]: URL_HASH_PARAMS_DEFAULT.queryString,
-    [HASH_PARAM_NAMES.QUERY_IS_CASE_SENSITIVE]: URL_HASH_PARAMS_DEFAULT.queryIsCaseSensitive,
+    [HASH_PARAM_NAMES.QUERY_IS_CASE_SENSITIVE]: 
+        URL_HASH_PARAMS_DEFAULT.queryIsCaseSensitive,
     [HASH_PARAM_NAMES.QUERY_IS_REGEX]: URL_HASH_PARAMS_DEFAULT.queryIsRegex,
 });

117-134: Well-implemented share button UI

The implementation of the share button in the title buttons area is clean and consistent with the existing UI patterns. The use of the ShareIcon and appropriate title provides good user experience.


138-144: Properly configured QueryInputBox with required props

The QueryInputBox is now correctly configured with all the necessary props, allowing it to function as a controlled component. This ensures that the search parameters are properly synchronized with the URL.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/services/decoders/ClpIrDecoder/index.ts (1)

46-53: Potential enhancement: add error handling for invalid data.
While the current logic is sound, consider gracefully handling the scenario where dataArray or parseFilterKeys might cause a parsing error. This helps avoid silent failures if corrupted data is encountered.

🛑 Comments failed to post (1)
src/services/decoders/ClpIrDecoder/index.ts (1)

104-107: ⚠️ Potential issue

Duplicate setFormatterOptions definitions detected.
It appears there are two methods named setFormatterOptions (one here and another at lines 122-129). This duplication may lead to compilation errors or confusion. Please unify them into a single method or remove the duplicate to avoid overshadowing.

Below is a suggested diff to remove the duplicate definition introduced later (lines 122-129), preserving the first one:

--- a/src/services/decoders/ClpIrDecoder/index.ts
+++ b/src/services/decoders/ClpIrDecoder/index.ts
@@ -121,9 +121,0 @@ class ClpIrDecoder implements Decoder {
-    setFormatterOptions (options: DecoderOptions): boolean {
-        this.#formatter = new YscopeFormatter({
-            formatString: options.formatString,
-            structuredIrNamespaceKeys: this.#structuredIrNamespaceKeys,
-        });
-
-        return true;
-    }
-

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.

2 participants