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

GUI menus and basic layout. #102

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

GUI menus and basic layout. #102

wants to merge 2 commits into from

Conversation

Jhuighuy
Copy link
Owner

@Jhuighuy Jhuighuy commented Nov 23, 2024

TODO:

  • Should we resize like in shadcn's UI?
  • Maybe factor-out common styles into a Tailwind config.
  • Unduplicate the code.
  • Tests.

TODO:

@Jhuighuy Jhuighuy force-pushed the main branch 6 times, most recently from bfae3fa to 6d7962f Compare November 23, 2024 23:40
@Jhuighuy Jhuighuy force-pushed the main branch 2 times, most recently from 553ebca to 6a7be63 Compare November 24, 2024 07:54
@Jhuighuy Jhuighuy marked this pull request as draft November 24, 2024 20:33
@Jhuighuy Jhuighuy force-pushed the main branch 2 times, most recently from ebed823 to 4705c88 Compare December 4, 2024 12:06
@Jhuighuy Jhuighuy force-pushed the main branch 3 times, most recently from 6d9aaee to 89bf409 Compare December 15, 2024 18:13
@Jhuighuy Jhuighuy self-assigned this Dec 16, 2024
@Jhuighuy Jhuighuy force-pushed the main branch 2 times, most recently from 1d00812 to 697a038 Compare January 10, 2025 09:37
@Jhuighuy Jhuighuy force-pushed the front-dev branch 2 times, most recently from 186ff15 to c6d73cb Compare February 7, 2025 08:18
@Jhuighuy Jhuighuy changed the title Some content for the frontend. GUI development. Feb 7, 2025
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 19.40928% with 191 lines in your changes missing coverage. Please review.

Project coverage is 82.07%. Comparing base (0fcf00e) to head (3486a7c).

Files with missing lines Patch % Lines
source/titfront/src/components/Menu.tsx 0.90% 109 Missing ⚠️
source/titfront/src/components/App.tsx 2.22% 44 Missing ⚠️
source/titfront/src/stores/LayoutStore.ts 0.00% 26 Missing ⚠️
source/titfront/src/components/Resizable.tsx 91.11% 4 Missing ⚠️
source/titfront/src/components/Viewer.tsx 20.00% 4 Missing ⚠️
source/titfront/src/main.tsx 0.00% 2 Missing ⚠️
source/titfront/src/utils.ts 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
- Coverage   84.91%   82.07%   -2.84%     
==========================================
  Files         114      119       +5     
  Lines        5124     5356     +232     
  Branches     1468     1478      +10     
==========================================
+ Hits         4351     4396      +45     
- Misses        648      835     +187     
  Partials      125      125              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Repository owner deleted a comment from codecov bot Feb 7, 2025
@Jhuighuy Jhuighuy force-pushed the front-dev branch 6 times, most recently from c806ad3 to ca07aba Compare February 13, 2025 00:01
@Jhuighuy Jhuighuy changed the title GUI development. GUI menus and basic layout. Feb 14, 2025
@Jhuighuy Jhuighuy force-pushed the front-dev branch 11 times, most recently from 52d0a6f to a35c67e Compare February 20, 2025 17:49
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
26.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@Jhuighuy
Copy link
Owner Author

@coderabbitai review

Copy link

coderabbitai bot commented Feb 20, 2025

Walkthrough

The pull request introduces several new components and updates within the application. A new App component was added to provide a structured layout with left and bottom menus, each containing multiple items paired with corresponding icons. Additionally, a new Menu component was implemented to manage menu interactions using React context and custom hooks, alongside its MenuItem subcomponent. The PythonShell component received a styling change that removed its gray background. A Resizable component was added with logic for dynamic resizing based on mouse interactions, accompanied by a dedicated test suite. A placeholder Viewer component was also introduced. The main entry point was updated to render the App component within PyConnectionProvider, replacing the previous PythonShell. Lastly, a Zustand-based menu state management system was implemented in the layout store, and utility exports for a side type and an array generator function were introduced.

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 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. (Beta)
  • @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: 11

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 0fcf00e and 3486a7c.

⛔ Files ignored due to path filters (1)
  • source/titfront/src/components/__snapshots__/PythonShell.test.tsx.snap is excluded by !**/*.snap
📒 Files selected for processing (9)
  • source/titfront/src/components/App.tsx (1 hunks)
  • source/titfront/src/components/Menu.tsx (1 hunks)
  • source/titfront/src/components/PythonShell.tsx (1 hunks)
  • source/titfront/src/components/Resizable.test.tsx (1 hunks)
  • source/titfront/src/components/Resizable.tsx (1 hunks)
  • source/titfront/src/components/Viewer.tsx (1 hunks)
  • source/titfront/src/main.tsx (1 hunks)
  • source/titfront/src/stores/LayoutStore.ts (1 hunks)
  • source/titfront/src/utils.ts (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
source/titfront/src/components/App.tsx

[warning] 7-8: source/titfront/src/components/App.tsx#L7-L8
Added lines #L7 - L8 were not covered by tests


[warning] 16-18: source/titfront/src/components/App.tsx#L16-L18
Added lines #L16 - L18 were not covered by tests


[warning] 22-22: source/titfront/src/components/App.tsx#L22
Added line #L22 was not covered by tests


[warning] 26-46: source/titfront/src/components/App.tsx#L26-L46
Added lines #L26 - L46 were not covered by tests


[warning] 48-55: source/titfront/src/components/App.tsx#L48-L55
Added lines #L48 - L55 were not covered by tests


[warning] 57-62: source/titfront/src/components/App.tsx#L57-L62
Added lines #L57 - L62 were not covered by tests


[warning] 64-66: source/titfront/src/components/App.tsx#L64-L66
Added lines #L64 - L66 were not covered by tests

source/titfront/src/components/Menu.tsx

[warning] 6-6: source/titfront/src/components/Menu.tsx#L6
Added line #L6 was not covered by tests


[warning] 16-16: source/titfront/src/components/Menu.tsx#L16
Added line #L16 was not covered by tests


[warning] 18-20: source/titfront/src/components/Menu.tsx#L18-L20
Added lines #L18 - L20 were not covered by tests


[warning] 34-34: source/titfront/src/components/Menu.tsx#L34
Added line #L34 was not covered by tests


[warning] 36-37: source/titfront/src/components/Menu.tsx#L36-L37
Added lines #L36 - L37 were not covered by tests


[warning] 47-47: source/titfront/src/components/Menu.tsx#L47
Added line #L47 was not covered by tests


[warning] 49-51: source/titfront/src/components/Menu.tsx#L49-L51
Added lines #L49 - L51 were not covered by tests


[warning] 54-55: source/titfront/src/components/Menu.tsx#L54-L55
Added lines #L54 - L55 were not covered by tests


[warning] 58-60: source/titfront/src/components/Menu.tsx#L58-L60
Added lines #L58 - L60 were not covered by tests


[warning] 63-63: source/titfront/src/components/Menu.tsx#L63
Added line #L63 was not covered by tests


[warning] 67-68: source/titfront/src/components/Menu.tsx#L67-L68
Added lines #L67 - L68 were not covered by tests


[warning] 70-70: source/titfront/src/components/Menu.tsx#L70
Added line #L70 was not covered by tests


[warning] 72-74: source/titfront/src/components/Menu.tsx#L72-L74
Added lines #L72 - L74 were not covered by tests


[warning] 76-102: source/titfront/src/components/Menu.tsx#L76-L102
Added lines #L76 - L102 were not covered by tests


[warning] 104-104: source/titfront/src/components/Menu.tsx#L104
Added line #L104 was not covered by tests


[warning] 106-109: source/titfront/src/components/Menu.tsx#L106-L109
Added lines #L106 - L109 were not covered by tests


[warning] 111-115: source/titfront/src/components/Menu.tsx#L111-L115
Added lines #L111 - L115 were not covered by tests


[warning] 117-119: source/titfront/src/components/Menu.tsx#L117-L119
Added lines #L117 - L119 were not covered by tests


[warning] 121-123: source/titfront/src/components/Menu.tsx#L121-L123
Added lines #L121 - L123 were not covered by tests


[warning] 125-125: source/titfront/src/components/Menu.tsx#L125
Added line #L125 was not covered by tests


[warning] 127-130: source/titfront/src/components/Menu.tsx#L127-L130
Added lines #L127 - L130 were not covered by tests


[warning] 132-139: source/titfront/src/components/Menu.tsx#L132-L139
Added lines #L132 - L139 were not covered by tests


[warning] 152-153: source/titfront/src/components/Menu.tsx#L152-L153
Added lines #L152 - L153 were not covered by tests

🪛 Biome (1.9.4)
source/titfront/src/components/Menu.tsx

[error] 37-37: Forbidden non-null assertion.

(lint/style/noNonNullAssertion)


[error] 113-116: Provide an explicit type prop for the button element.

The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset

(lint/a11y/useButtonType)


[error] 166-173: Provide an explicit type prop for the button element.

The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset

(lint/a11y/useButtonType)

source/titfront/src/components/Resizable.tsx

[error] 68-73: Provide an explicit type prop for the button element.

The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset

(lint/a11y/useButtonType)

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: Build (macos-15, clang++-19, Release)
  • GitHub Check: Build (macos-15, clang++-19, Debug)
  • GitHub Check: Build (macos-15, g++-14, Release)
  • GitHub Check: Build (macos-15, g++-14, Debug)
  • GitHub Check: Build (ubuntu-24.04, clang++-19, Release)
  • GitHub Check: Build (ubuntu-24.04, clang++-19, Debug)
  • GitHub Check: Build (ubuntu-24.04, g++-14, Release)
  • GitHub Check: Build (ubuntu-24.04, g++-14, Debug)
🔇 Additional comments (6)
source/titfront/src/main.tsx (1)

9-9: LGTM!

The changes correctly integrate the new App component while maintaining the necessary provider structure.

Also applies to: 18-18

source/titfront/src/stores/LayoutStore.ts (1)

19-44: Add tests for the menu store.

To align with PR objectives, consider adding tests for:

  • Size validation and constraints
  • Active item toggle behavior
  • Store instance creation and access

Would you like me to help generate the test suite?

source/titfront/src/utils.ts (1)

11-14: LGTM!

The Side type is well-documented and correctly defines the possible values.

source/titfront/src/components/App.tsx (1)

1-71: Add tests for the App component.

The component lacks test coverage. Consider adding tests to verify the rendering of menus and their interactions.

Would you like me to help generate test cases for the App component?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 7-8: source/titfront/src/components/App.tsx#L7-L8
Added lines #L7 - L8 were not covered by tests


[warning] 16-18: source/titfront/src/components/App.tsx#L16-L18
Added lines #L16 - L18 were not covered by tests


[warning] 22-22: source/titfront/src/components/App.tsx#L22
Added line #L22 was not covered by tests


[warning] 26-46: source/titfront/src/components/App.tsx#L26-L46
Added lines #L26 - L46 were not covered by tests


[warning] 48-55: source/titfront/src/components/App.tsx#L48-L55
Added lines #L48 - L55 were not covered by tests


[warning] 57-62: source/titfront/src/components/App.tsx#L57-L62
Added lines #L57 - L62 were not covered by tests


[warning] 64-66: source/titfront/src/components/App.tsx#L64-L66
Added lines #L64 - L66 were not covered by tests

source/titfront/src/components/Resizable.test.tsx (1)

15-121: Well-structured and comprehensive test suite!

The test suite thoroughly covers the component's functionality including:

  • Rendering children
  • Initial dimensions
  • Mouse interactions
  • Size constraints
source/titfront/src/components/Menu.tsx (1)

1-192: Add tests for the Menu component.

The component lacks test coverage. Consider adding tests to verify:

  • Menu item rendering and activation
  • Context provider functionality
  • Custom actions
  • Resizing integration

Would you like me to help generate test cases for the Menu component?

🧰 Tools
🪛 Biome (1.9.4)

[error] 37-37: Forbidden non-null assertion.

(lint/style/noNonNullAssertion)


[error] 113-116: Provide an explicit type prop for the button element.

The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset

(lint/a11y/useButtonType)


[error] 166-173: Provide an explicit type prop for the button element.

The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset

(lint/a11y/useButtonType)

🪛 GitHub Check: codecov/patch

[warning] 6-6: source/titfront/src/components/Menu.tsx#L6
Added line #L6 was not covered by tests


[warning] 16-16: source/titfront/src/components/Menu.tsx#L16
Added line #L16 was not covered by tests


[warning] 18-20: source/titfront/src/components/Menu.tsx#L18-L20
Added lines #L18 - L20 were not covered by tests


[warning] 34-34: source/titfront/src/components/Menu.tsx#L34
Added line #L34 was not covered by tests


[warning] 36-37: source/titfront/src/components/Menu.tsx#L36-L37
Added lines #L36 - L37 were not covered by tests


[warning] 47-47: source/titfront/src/components/Menu.tsx#L47
Added line #L47 was not covered by tests


[warning] 49-51: source/titfront/src/components/Menu.tsx#L49-L51
Added lines #L49 - L51 were not covered by tests


[warning] 54-55: source/titfront/src/components/Menu.tsx#L54-L55
Added lines #L54 - L55 were not covered by tests


[warning] 58-60: source/titfront/src/components/Menu.tsx#L58-L60
Added lines #L58 - L60 were not covered by tests


[warning] 63-63: source/titfront/src/components/Menu.tsx#L63
Added line #L63 was not covered by tests


[warning] 67-68: source/titfront/src/components/Menu.tsx#L67-L68
Added lines #L67 - L68 were not covered by tests


[warning] 70-70: source/titfront/src/components/Menu.tsx#L70
Added line #L70 was not covered by tests


[warning] 72-74: source/titfront/src/components/Menu.tsx#L72-L74
Added lines #L72 - L74 were not covered by tests


[warning] 76-102: source/titfront/src/components/Menu.tsx#L76-L102
Added lines #L76 - L102 were not covered by tests


[warning] 104-104: source/titfront/src/components/Menu.tsx#L104
Added line #L104 was not covered by tests


[warning] 106-109: source/titfront/src/components/Menu.tsx#L106-L109
Added lines #L106 - L109 were not covered by tests


[warning] 111-115: source/titfront/src/components/Menu.tsx#L111-L115
Added lines #L111 - L115 were not covered by tests


[warning] 117-119: source/titfront/src/components/Menu.tsx#L117-L119
Added lines #L117 - L119 were not covered by tests


[warning] 121-123: source/titfront/src/components/Menu.tsx#L121-L123
Added lines #L121 - L123 were not covered by tests


[warning] 125-125: source/titfront/src/components/Menu.tsx#L125
Added line #L125 was not covered by tests


[warning] 127-130: source/titfront/src/components/Menu.tsx#L127-L130
Added lines #L127 - L130 were not covered by tests


[warning] 132-139: source/titfront/src/components/Menu.tsx#L132-L139
Added lines #L132 - L139 were not covered by tests


[warning] 152-153: source/titfront/src/components/Menu.tsx#L152-L153
Added lines #L152 - L153 were not covered by tests

Comment on lines +10 to +16
export const Viewer: FC = () => {
return (
<div className="size-full flex items-center justify-center bg-black">
<span className="text-white">Viewer (placeholder)</span>
</div>
);
};
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Add tests and improve accessibility.

While the placeholder implementation is good, consider the following improvements:

  1. Add unit tests to align with PR objectives.
  2. Add ARIA attributes for better accessibility.
 export const Viewer: FC = () => {
   return (
-    <div className="size-full flex items-center justify-center bg-black">
-      <span className="text-white">Viewer (placeholder)</span>
+    <div 
+      className="size-full flex items-center justify-center bg-black"
+      role="region"
+      aria-label="Content viewer"
+    >
+      <span className="text-white" aria-live="polite">
+        Viewer (placeholder)
+      </span>
     </div>
   );
 };
📝 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.

Suggested change
export const Viewer: FC = () => {
return (
<div className="size-full flex items-center justify-center bg-black">
<span className="text-white">Viewer (placeholder)</span>
</div>
);
};
export const Viewer: FC = () => {
return (
<div
className="size-full flex items-center justify-center bg-black"
role="region"
aria-label="Content viewer"
>
<span className="text-white" aria-live="polite">
Viewer (placeholder)
</span>
</div>
);
};

Comment on lines +22 to +25
setSize(size) {
assert(size > 0, "Menu size must be positive!");
set({ size });
},
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Add size constraints for better UX.

Consider adding minimum and maximum size constraints to prevent the menu from becoming too small or too large.

 setSize(size) {
   assert(size > 0, "Menu size must be positive!");
+  const minSize = 200;
+  const maxSize = window.innerWidth * 0.8;
+  const constrainedSize = Math.min(Math.max(size, minSize), maxSize);
-  set({ size });
+  set({ size: constrainedSize });
 },
📝 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.

Suggested change
setSize(size) {
assert(size > 0, "Menu size must be positive!");
set({ size });
},
setSize(size) {
assert(size > 0, "Menu size must be positive!");
const minSize = 200;
const maxSize = window.innerWidth * 0.8;
const constrainedSize = Math.min(Math.max(size, minSize), maxSize);
set({ size: constrainedSize });
},

Comment on lines +30 to +35
/**
* Generate an array of numbers from `0` to `n - 1`.
*/
export function iota(n: number): number[] {
return Array.from({ length: n }, (_, i) => i);
}
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Optimize iota for large arrays.

Consider using a more performant implementation for large arrays.

 export function iota(n: number): number[] {
+  // For small arrays, current implementation is fine
+  if (n <= 1000) {
   return Array.from({ length: n }, (_, i) => i);
+  }
+  // For large arrays, use traditional for loop
+  const result = new Array(n);
+  for (let i = 0; i < n; i++) {
+    result[i] = i;
+  }
+  return result;
 }

Comment on lines +23 to +24
const leftIconSize = 24;
const bottomIconSize = 16;
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider extracting icon sizes into constants or theme configuration.

The hardcoded icon sizes (leftIconSize = 24 and bottomIconSize = 16) should be moved to a theme configuration to maintain consistency across the application.

+// In a new file: source/titfront/src/theme/constants.ts
+export const ICON_SIZES = {
+  menuLeft: 24,
+  menuBottom: 16,
+} as const;

-  const leftIconSize = 24;
-  const bottomIconSize = 16;
+  const { menuLeft: leftIconSize, menuBottom: bottomIconSize } = ICON_SIZES;

Also applies to: 25-25

Comment on lines +27 to +48
<Menu side="left">
<Menu.Item
name="Configuration"
icon={<SlidersIcon size={leftIconSize} />}
group={0}
/>
<Menu.Item
name="Storage"
icon={<DatabaseIcon size={leftIconSize} />}
group={0}
/>
<Menu.Item
name="Activity"
icon={<ActivityIcon size={leftIconSize} />}
group={1}
/>
<Menu.Item
name="Settings"
icon={<SettingsIcon size={leftIconSize} />}
group={1}
/>
</Menu>
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider extracting menu items configuration.

The menu items are currently hardcoded in the component. Consider extracting them into a configuration object for better maintainability and reusability.

+// In a new file: source/titfront/src/config/menuConfig.ts
+import { IconType } from 'react-icons';
+import { SlidersIcon, DatabaseIcon, ActivityIcon, SettingsIcon } from "react-icons/fi";
+
+interface MenuItem {
+  name: string;
+  icon: IconType;
+  group: number;
+}
+
+export const LEFT_MENU_ITEMS: MenuItem[] = [
+  { name: "Configuration", icon: SlidersIcon, group: 0 },
+  { name: "Storage", icon: DatabaseIcon, group: 0 },
+  { name: "Activity", icon: ActivityIcon, group: 1 },
+  { name: "Settings", icon: SettingsIcon, group: 1 },
+];

Also applies to: 51-65

Comment on lines +113 to +121
<button
className={index === activeItem ? activeItemCn : itemCn}
onClick={() => setActiveItem(index)}
>
{item.props.icon}
{horizontal || (
<span className="ml-1">{item.props.name}</span>
)}
</button>
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add type attributes to button elements.

The button elements should have explicit type attributes to prevent form submission behavior.

 <button
   className={index === activeItem ? activeItemCn : itemCn}
   onClick={() => setActiveItem(index)}
+  type="button"
 >

 <button
   key={key}
   className={cn(
     "w-6 h-6 mr-2 rounded flex items-center justify-center",
     "hover:bg-gray-500"
   )}
   onClick={callback}
+  type="button"
 >

Also applies to: 166-176

🧰 Tools
🪛 Biome (1.9.4)

[error] 113-116: Provide an explicit type prop for the button element.

The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset

(lint/a11y/useButtonType)

🪛 GitHub Check: codecov/patch

[warning] 117-119: source/titfront/src/components/Menu.tsx#L117-L119
Added lines #L117 - L119 were not covered by tests

}: MenuProps) => {
const { size, setSize, activeItem, setActiveItem } = useMenuStore(side);

/** @todo We should have a way to add custom actions from the children. */
Copy link

@coderabbitai coderabbitai bot Feb 20, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Address the TODO comment about custom actions.

The comment indicates a need for custom actions from children. Consider implementing this feature or creating an issue to track it.

Would you like me to help implement custom actions support or create an issue to track this task?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, please, make a sketch on how this can be done. I want the actions to be added from withing the child of a Menu.Item.

Copy link

Choose a reason for hiding this comment

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

@Jhuighuy Here's a sketch of how we can implement custom actions from children:

// Modified context interface
interface MenuContextProps {
  actions: Action[];
  registerAction: (action: Action) => void;
  unregisterAction: (key: string) => void;
}

// Updated Menu component
export const Menu: FC<MenuProps> & { Item: FC<MenuItemProps> } = ({
  side,
  children = [],
}: MenuProps) => {
  const { size, setSize, activeItem, setActiveItem } = useMenuStore(side);
  const [customActions, setCustomActions] = useState<Action[]>([]);

  const registerAction = useCallback((action: Action) => {
    setCustomActions(prev => [...prev, action]);
  }, []);

  const unregisterAction = useCallback((key: string) => {
    setCustomActions(prev => prev.filter(action => action.key !== key));
  }, []);

  const contextValue = useMemo(() => ({
    actions: [
      // Default minimize action
      {
        key: "minimize",
        icon: <MinimizeIcon />,
        callback: () => setActiveItem(-1),
      },
      ...customActions,
    ],
    registerAction,
    unregisterAction,
  }), [customActions, setActiveItem]);

  return (
    <MenuContext.Provider value={contextValue}>
      {/* ... rest of the component ... */}
    </MenuContext.Provider>
  );
};

// Example usage in a child component
const MyCustomMenuItem = () => {
  const { registerAction, unregisterAction } = useMenu();

  useEffect(() => {
    const action = {
      key: "custom-action",
      icon: <CustomIcon />,
      callback: () => {
        // Custom action logic
      },
    };
    registerAction(action);
    return () => unregisterAction(action.key);
  }, []);

  return <div>Custom Menu Item</div>;
};

// Usage
<Menu side="left">
  <Menu.Item>
    <MyCustomMenuItem />
  </Menu.Item>
</Menu>

This approach:

  1. Allows children to register/unregister actions dynamically
  2. Maintains type safety
  3. Cleans up actions when components unmount
  4. Preserves the default minimize action

Would you like me to explain any part in more detail?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Can we encapsulate the logic of registering and unregistering into a custom hook?

Copy link

Choose a reason for hiding this comment

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

@Jhuighuy Here's how we can encapsulate the logic into a custom hook:

// Custom hook for action management
const useMenuAction = (action: Action) => {
  const { registerAction, unregisterAction } = useMenu();
  
  useEffect(() => {
    registerAction(action);
    return () => unregisterAction(action.key);
  }, [action]);
};

// Example usage in a child component
const MyCustomMenuItem = () => {
  useMenuAction({
    key: "custom-action",
    icon: <CustomIcon />,
    callback: () => {
      // Custom action logic
    },
  });

  return <div>Custom Menu Item</div>;
};

This simplifies the usage and ensures consistent cleanup. Would you like me to show how to handle dynamic action updates as well?

Comment on lines +36 to +38
export function useMenu() {
return useContext(MenuContext)!;
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove non-null assertion and add error handling.

The useMenu hook uses a non-null assertion which could lead to runtime errors if the context is null.

 export function useMenu() {
-  return useContext(MenuContext)!;
+  const context = useContext(MenuContext);
+  if (context === null) {
+    throw new Error("useMenu must be used within a MenuContext.Provider");
+  }
+  return context;
 }
📝 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.

Suggested change
export function useMenu() {
return useContext(MenuContext)!;
}
export function useMenu() {
const context = useContext(MenuContext);
if (context === null) {
throw new Error("useMenu must be used within a MenuContext.Provider");
}
return context;
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 37-37: Forbidden non-null assertion.

(lint/style/noNonNullAssertion)

🪛 GitHub Check: codecov/patch

[warning] 36-37: source/titfront/src/components/Menu.tsx#L36-L37
Added lines #L36 - L37 were not covered by tests

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.

1 participant