DT-93: Refactor and fix Project Page Settings #93
Conversation
abdotop
changed the title
…eployment management and token regeneration, update `GET/api/deployment` input, and refine UI layout.
…and introduce new reusable UI components.
There was a problem hiding this comment.
Pull request overview
This PR refactors the project settings page by relocating and reimplementing the SettingsPage component, while also fixing API route definitions.
Changes:
- Moved SettingsPage from
web/pages/project/SettingsPage.tsxtoweb/pages/SettingsPage.tsxwith complete UI redesign - Updated API routes to correct HTTP methods and parameter structures
- Added layout improvements for proper overflow handling
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web/pages/project/SettingsPage.tsx | Complete removal of old SettingsPage implementation |
| web/pages/SettingsPage.tsx | New SettingsPage with redesigned UI, deployment management, and token handling |
| web/pages/ProjectPage.tsx | Updated import path and added overflow handling classes |
| web/pages/DeploymentPage.tsx | Added sidebar item check to logs fetching logic |
| web/index.tsx | Enhanced main layout with flexbox and overflow classes |
| api/routes.ts | Fixed API route descriptions, HTTP methods, and parameter structures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn: async (_ctx, { url }) => { | ||
| const dep = DeploymentsCollection.get(url) |
There was a problem hiding this comment.
The parameter name 'url' shadows the imported 'url' from '@01edu/signal-router'. Consider renaming this parameter to 'deploymentUrl' or 'depUrl' to avoid confusion.
| fn: async (_ctx, { url }) => { | |
| const dep = DeploymentsCollection.get(url) | |
| fn: async (_ctx, { url: deploymentUrl }) => { | |
| const dep = DeploymentsCollection.get(deploymentUrl) |
| const projectId = project.data?.slug | ||
| if (!projectId) return | ||
| try { | ||
| const depUrl = new URL(fd.get('url') as string).host |
There was a problem hiding this comment.
If the URL is malformed, this will throw a generic TypeError. Consider wrapping this in a try-catch and providing a clearer error message like 'Invalid URL format' to improve user experience.
| replace: true, | ||
| }) | ||
| } catch (e) { | ||
| addDeploymentError.value = e instanceof Error ? e.message : 'Unknown error' |
There was a problem hiding this comment.
The error message 'Unknown error' is not descriptive. Consider using a more specific message such as 'Failed to add deployment' to provide better context to users.
| addDeploymentError.value = e instanceof Error ? e.message : 'Unknown error' | |
| addDeploymentError.value = e instanceof Error | |
| ? e.message | |
| : 'Failed to add deployment' |
| <select | ||
| class='select select-bordered select-sm w-full max-w-xs' | ||
| value={selectedUrl || ''} | ||
| defaultValue={selectedUrl || ''} |
There was a problem hiding this comment.
Both 'value' and 'defaultValue' are set on the same select element. The 'value' prop makes this a controlled component, so 'defaultValue' is redundant and will be ignored. Remove the 'defaultValue' prop.
| defaultValue={selectedUrl || ''} |
2 participants
refactor: reimplement SettingsPage and adjust API routes.