Feat/window wrapper by christopher-ling · Pull Request #5 · christopher-ling/MacOS-Portfolio

christopher-ling · 2025-12-22T03:50:53Z

No description provided.

Copilot

Pull request overview

This PR implements window management functionality including minimize and resize capabilities for window components. The changes enhance the window wrapper HOC with interactive resize handles, update the state management to track minimized state and dimensions, and refactor the Terminal component's structure with improved CSS organization.

Key changes:

Added window minimize and resize functionality with state management
Implemented interactive resize handles with mouse event handlers
Refactored Terminal component structure with semantic CSS class names

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
src/hoc/WindowWrapper.jsx	Adds resize functionality with mouse event handlers and state for window dimensions; updates draggable trigger to use window header
src/store/window.js	Adds minimizeWindow and resizeWindow actions; updates openWindow/closeWindow to manage isMinimized, width, and height state
src/constants/index.js	Extends window configuration objects to include isMinimized, width, and height properties
src/components/WindowControls.jsx	Connects minimize button to minimizeWindow action
src/components/Dock.jsx	Implements window restore logic for minimized windows and adds visual indicator dot for open windows
src/windows/Terminal.jsx	Restructures component with nested divs and more semantic class names for better styling control
src/index.css	Adds terminal content/body styles, resize handle styles, and nav z-index for proper layering

Comments suppressed due to low confidence (1)

src/hoc/WindowWrapper.jsx:26

Unused variable isResizing.

        const [isResizing, setIsResizing] = useState(false);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/hoc/WindowWrapper.jsx

Copilot · 2025-12-22T03:53:57Z

src/hoc/WindowWrapper.jsx

+                        newWidth = Math.max(300, startWidth + (e.clientX - startX));
+                    }
+                    if (edge.includes('left')) {
+                        const delta = e.clientX - startX;
+                        newWidth = Math.max(300, startWidth - delta);
+                        if (newWidth > 300) newLeft = startLeft + delta;
+                    }
+                    if (edge.includes('bottom')) {
+                        newHeight = Math.max(200, startHeight + (e.clientY - startY));
+                    }
+                    if (edge.includes('top')) {
+                        const delta = e.clientY - startY;
+                        newHeight = Math.max(200, startHeight - delta);
+                        if (newHeight > 200) newTop = startTop + delta;


The minimum width and height values (300 and 200) are hardcoded in multiple places. These magic numbers should be extracted into named constants at the top of the file or imported from a configuration file to improve maintainability and make it easier to adjust these values consistently.

@copilot open a new pull request to apply changes based on this feedback

src/hoc/WindowWrapper.jsx

src/store/window.js

src/index.css

Copilot · 2025-12-22T03:53:58Z

src/hoc/WindowWrapper.jsx

+        const { focusWindow, windows, resizeWindow } = useWindowStore();
+        const { isOpen, zIndex, isMinimized, width, height } = windows[windowKey];
        const ref = useRef(null);
+        const [isResizing, setIsResizing] = useState(false);


The isResizing state variable is set but never used anywhere in the component. This is unused state that adds unnecessary overhead. Consider removing it or implementing the intended functionality.

Copilot · 2025-12-22T03:53:58Z

src/hoc/WindowWrapper.jsx

+                        newHeight = Math.max(200, startHeight - delta);
+                        if (newHeight > 200) newTop = startTop + delta;


When resizing from the top edge, the position update logic has a bug. The condition checks if newHeight is greater than 200, but it should check if the calculated height before clamping would have been greater than 200. This causes the window position to not update correctly when the height is at the minimum constraint, resulting in jerky resize behavior at the minimum height boundary.

Suggested change

newHeight = Math.max(200, startHeight - delta);

if (newHeight > 200) newTop = startTop + delta;

const rawHeight = startHeight - delta;

newHeight = Math.max(200, rawHeight);

if (rawHeight > 200) newTop = startTop + delta;

Copilot · 2025-12-22T03:53:58Z

src/components/Dock.jsx

+                        {windows[id]?.isOpen && (
+                            <div className="absolute -bottom-1 w-1 h-1 bg-white rounded-full" />
+                        )}


The indicator dot is shown when isOpen is true, but it should also consider the isMinimized state. According to typical window management UX patterns, the indicator should remain visible for minimized windows. However, verify if this matches the intended behavior - if minimized windows should not show the indicator, then the condition should be isOpen && !isMinimized.

src/store/window.js

Copilot · 2025-12-22T03:58:52Z

@christopher-ling I've opened a new pull request, #6, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/hoc/WindowWrapper.jsx:26

Unused variable isResizing.

        const [isResizing, setIsResizing] = useState(false);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-22T04:03:53Z

src/hoc/WindowWrapper.jsx

+                        newWidth = Math.max(300, startWidth + (e.clientX - startX));
+                    }
+                    if (edge.includes('left')) {
+                        const delta = e.clientX - startX;
+                        const proposedWidth = startWidth - delta;
+                        newWidth = Math.max(300, proposedWidth);
+                        if (proposedWidth > 300) newLeft = startLeft + delta;
+                    }
+                    if (edge.includes('bottom')) {
+                        newHeight = Math.max(200, startHeight + (e.clientY - startY));
+                    }
+                    if (edge.includes('top')) {
+                        const delta = e.clientY - startY;
+                        const proposedHeight = startHeight - delta;
+                        newHeight = Math.max(200, proposedHeight);
+                        if (proposedHeight > 200) newTop = startTop + delta;
+                    }


The minimum width (300) and minimum height (200) are magic numbers that appear multiple times in the resize logic. Consider extracting these as named constants at the top of the file or component for better maintainability and consistency.

Copilot · 2025-12-22T04:03:53Z

src/hoc/WindowWrapper.jsx

+            const [instance] = Draggable.create(el, { 
+                trigger: el.querySelector("#window-header"),


The Draggable trigger queries for '#window-header' on mount, but if the Component is not yet rendered or doesn't contain this element, the querySelector will return null and dragging may not work. Consider adding error handling or validation to ensure the trigger element exists before creating the Draggable instance.

Suggested change

const [instance] = Draggable.create(el, {

trigger: el.querySelector("#window-header"),

const headerEl = el.querySelector("#window-header");

if (!headerEl) return;

const [instance] = Draggable.create(el, {

trigger: headerEl,

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

src/hoc/WindowWrapper.jsx:26

Unused variable isResizing.

        const [isResizing, setIsResizing] = useState(false);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-22T04:09:25Z

src/hoc/WindowWrapper.jsx

+                <div className="resize-right" />
+                <div className="resize-left" />
+                <div className="resize-bottom" />
+                <div className="resize-top" />
+                <div className="resize-corner-br" />
+                <div className="resize-corner-bl" />
+                <div className="resize-corner-tr" />
+                <div className="resize-corner-tl" />


The resize handles are not accessible via keyboard navigation. Users who rely on keyboard-only navigation cannot resize windows. Consider adding tabindex, role, aria-label attributes, and keyboard event handlers (e.g., arrow keys) to make these handles accessible.

Copilot · 2025-12-22T04:09:25Z

src/components/WindowControls.jsx

    <div className="close" onClick={() => closeWindow(target)}/>
-    <div className="minimize" />
+    <div className="minimize" onClick={() => minimizeWindow(target)}/>
    <div className="maximize" />


The window control buttons (close, minimize, maximize) lack accessibility attributes. They should have aria-label or title attributes to describe their function for screen reader users, and should use semantic button elements instead of divs with onClick handlers.

Copilot · 2025-12-22T04:09:26Z

src/hoc/WindowWrapper.jsx

+            edges.forEach(({ selector, edge }) => {
+                const handle = el.querySelector(selector);
+                if (handle) {
+                    handle.addEventListener('mousedown', (e) => handleMouseDown(e, edge));
+                }
+            });


The event listeners added to resize handles are never removed, causing a memory leak. Each time the component re-renders and the useLayoutEffect runs (when dependencies change), new event listeners are added without cleaning up the previous ones. A cleanup function should be returned that removes all added event listeners.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2025-12-22T04:09:26Z

src/index.css


  nav {
-    @apply flex justify-between items-center bg-white/50 backdrop-blur-3xl p-2 px-5 select-none;
+    @apply flex justify-between items-center bg-white/50 backdrop-blur-3xl p-2 px-5 select-none relative z-40;


The z-index value of 40 for the nav is hardcoded here, but window z-indexes start at INITIAL_Z_INDEX (1000) and increment from there. This could cause windows to appear above the navigation bar. Consider using a z-index value higher than the maximum possible window z-index, or defining navigation z-index as a constant.

Suggested change

@apply flex justify-between items-center bg-white/50 backdrop-blur-3xl p-2 px-5 select-none relative z-40;

@apply flex justify-between items-center bg-white/50 backdrop-blur-3xl p-2 px-5 select-none relative z-[9999];

- Implement minimize window feature with white dot indicator in dock - Add maximize functionality triggered by green button or drag-to-top - Add navbar highlight effect when dragging window to top - Make only window header draggable (not entire window) - Maximized windows cover dock and fill screen below navbar - Windows reset to original size when closed - Fix minimize restore behavior in dock click handler

Copilot · 2025-12-22T04:56:08Z

@christopher-ling I've opened a new pull request, #7, to work on those changes. Once the pull request is ready, I'll request review from you.

christopher-ling added 2 commits December 21, 2025 22:35

minimize functionality

aff4cda

close improvement and draggable sizing

63767c2

christopher-ling requested a review from Copilot December 22, 2025 03:50

Copilot started reviewing on behalf of christopher-ling December 22, 2025 03:51 View session

Copilot AI reviewed Dec 22, 2025

View reviewed changes

Copilot AI mentioned this pull request Dec 22, 2025

Extract hardcoded window size constraints to named constants #6

Closed

christopher-ling requested a review from Copilot December 22, 2025 04:00

Copilot started reviewing on behalf of christopher-ling December 22, 2025 04:01 View session

Copilot AI reviewed Dec 22, 2025

View reviewed changes

christopher-ling force-pushed the feat/window-wrapper branch from fe42c09 to 63767c2 Compare December 22, 2025 04:04

christopher-ling requested review from Copilot December 22, 2025 04:06

Copilot started reviewing on behalf of christopher-ling December 22, 2025 04:06 View session

Copilot AI reviewed Dec 22, 2025

View reviewed changes

christopher-ling added 2 commits December 21, 2025 23:37

cursor pointers

52a4f18

Copilot AI mentioned this pull request Dec 22, 2025

Resolve memory leak feedback - issue already fixed in prior refactor #7

Closed

christopher-ling merged commit ee978dd into main Dec 22, 2025

		newHeight = Math.max(200, startHeight - delta);
		if (newHeight > 200) newTop = startTop + delta;

-                        newHeight = Math.max(200, startHeight - delta);
-                        if (newHeight > 200) newTop = startTop + delta;
+                        const rawHeight = startHeight - delta;
+                        newHeight = Math.max(200, rawHeight);
+                        if (rawHeight > 200) newTop = startTop + delta;

		const [instance] = Draggable.create(el, {
		trigger: el.querySelector("#window-header"),

	@apply flex justify-between items-center bg-white/50 backdrop-blur-3xl p-2 px-5 select-none relative z-40;
	@apply flex justify-between items-center bg-white/50 backdrop-blur-3xl p-2 px-5 select-none relative z-[9999];

Conversation

christopher-ling commented Dec 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

christopher-ling Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Dec 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

christopher-ling Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Dec 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants