-
Notifications
You must be signed in to change notification settings - Fork 2
feat: project config view #1372
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
base: main
Are you sure you want to change the base?
Conversation
05d3206 to
246d041
Compare
73ede8d to
1b3d88b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This is a major refactoring PR that restructures the project navigation and configuration UI from a tab-based system to a drawer/sidebar-based architecture. The changes rename "Code & Assets" to "Files/Explorer", consolidate settings into a drawer, and improve the overall navigation experience.
Key Changes:
- Migrated from tab-based navigation to a drawer-based sidebar architecture for project settings
- Renamed "Code & Assets" to "Files" throughout the codebase
- Replaced global drawer store with project-scoped drawer state in
useSharedBetweenProjectsStore - Added new utility functions for navigation with settings preservation
Reviewed Changes
Copilot reviewed 189 out of 197 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/validations/coseAndAssets.schema.ts | Renamed schema from codeAssetsSchema to codeFilesSchema |
| src/validations/index.ts | Updated exports to use new schema name |
| src/store/useModalStore.ts | Enhanced modal store with per-modal data storage and getter function |
| src/store/useSharedBetweenProjectsStore.ts | Replaced chatbot-specific state with generic drawer management system |
| src/store/useProjectStore.ts | Removed latestOpened tracking state |
| src/utilities/navigation.ts | Added new navigation utilities for settings path preservation |
| src/utilities/fileTree.utils.ts | New utilities for building file trees and calculating optimal widths |
| src/components/organisms/projectSettingsView/* | New drawer-based project settings components |
| src/components/organisms/projectFiles/* | New file explorer sidebar component |
| src/routes.tsx | Updated routing structure to support settings drawer |
| src/locales/en/* | Updated translation keys from "code&assets" to "files" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| modals: { | ||
| [ModalName.editConnection]: true, | ||
| }, |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The modal is set to forceOpen with [ModalName.editConnection]: true in the initial state. This means the edit connection modal will be open by default on application start, which is likely unintended behavior.
| if (resources && Object.values(resources || {}).length && !isLoadingCode && fileToOpen && !fileToOpenIsOpened) { | ||
| if ( | ||
| resources && | ||
| Object.values(resources || {}).length && |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This use of variable 'resources' always evaluates to true.
| }); | ||
| LoggerService.error(namespaces.sessionsService, `${t("executionFailedExtended", { projectId, error })}`); | ||
| closeDrawer(DrawerName.projectManualRunSettings); | ||
| if (projectId) { |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This use of variable 'projectId' always evaluates to true.
| type: "success", | ||
| }); | ||
| closeDrawer(DrawerName.projectManualRunSettings); | ||
| if (projectId) { |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This use of variable 'projectId' always evaluates to true.
7ed3701 to
bf76484
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 226 out of 398 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const connectionStatusCheckInterval = 1500; | ||
| export const maxConnectionsCheckRetries = 600; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The retry count has been increased from 60 to 600, which could result in connection checks running for 15 minutes (600 × 1500ms). This is a significant increase that could lead to longer wait times and resource consumption. Consider whether this extended retry duration aligns with user experience expectations and whether a timeout mechanism should be documented.
| export const connectionStatusCheckInterval = 1500; | |
| export const maxConnectionsCheckRetries = 600; | |
| export const connectionStatusCheckInterval = 1500; | |
| // Maximum number of retries for connection status checks. With the current interval, this could result in up to 15 minutes of waiting (600 × 1500ms). | |
| // Consider using connectionStatusCheckTimeout to limit the total wait time. | |
| export const maxConnectionsCheckRetries = 600; | |
| // Maximum total timeout for connection status checks (e.g., 2 minutes). | |
| export const connectionStatusCheckTimeout = 120000; |
| useEffect(() => { | ||
| if (!trigger) return; | ||
|
|
||
| const configureTriggerDisplay = async (trigger: Trigger) => { |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function configureTriggerDisplay is defined inside useEffect but lacks proper dependency tracking. The baseDetails dependency in the useEffect at line 83 is itself a useMemo that depends on trigger?.path and trigger?.entryFunction, which can cause the effect to run unnecessarily. Consider moving configureTriggerDisplay outside the effect or memoizing it with useCallback.
| const nameClassName = cn("text-gray-300 placeholder:text-gray-1100", dirtyFields["name"] ? "border-white" : ""); | ||
| const valueClassName = cn("text-gray-300 placeholder:text-gray-1100", dirtyFields["value"] ? "border-white" : ""); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These className computations are recalculated on every render. Since they depend on dirtyFields, consider using useMemo to avoid unnecessary recalculations, especially if the component re-renders frequently.
| const nameClassName = cn("text-gray-300 placeholder:text-gray-1100", dirtyFields["name"] ? "border-white" : ""); | |
| const valueClassName = cn("text-gray-300 placeholder:text-gray-1100", dirtyFields["value"] ? "border-white" : ""); | |
| const nameClassName = React.useMemo( | |
| () => cn("text-gray-300 placeholder:text-gray-1100", dirtyFields["name"] ? "border-white" : ""), | |
| [dirtyFields["name"]] | |
| ); | |
| const valueClassName = React.useMemo( | |
| () => cn("text-gray-300 placeholder:text-gray-1100", dirtyFields["value"] ? "border-white" : ""), | |
| [dirtyFields["value"]] | |
| ); |
| (selectIntegrationSlack as SelectOption[]).splice( | ||
| selectIntegrationSlack.findIndex((opt) => opt.value === ConnectionAuthType.OauthDefault), | ||
| 1, | ||
| { label: "OAuth v2 - Default app", value: ConnectionAuthType.Oauth } | ||
| ); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directly mutating the imported selectIntegrationSlack array can cause unexpected side effects across the application, as this constant is shared. This mutation will persist across component re-renders and affect all uses of this constant. Create a new array instead of mutating the original.
| setIsInitialLoad(false); | ||
|
|
||
| if (!nextPageToken && data.sessions.length > 0) { | ||
| if (firstTimeLoading && !nextPageToken && data.sessions.length > 0) { |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition checks firstTimeLoading but this variable is set to false inside the same conditional block at line 243. This creates a dependency on the order of execution and could lead to unexpected behavior if the logic changes. Consider restructuring to make the state transition more explicit.
| const fetchData = useCallback(async () => { | ||
| setIsSourceLoad(true); | ||
| await fetchEvents(true, sourceId, projectId); | ||
| await fetchEvents({ projectId, sourceId }); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fetchEvents call signature has changed from fetchEvents(true, sourceId, projectId) to fetchEvents({ projectId, sourceId }), but the implementation now passes an object instead of positional parameters. Ensure all callers of fetchEvents have been updated to use the new signature to avoid runtime errors.
[## Description
Linear Ticket
What type of PR is this? (check all applicable)
](feat: project config view)feat: project config view