-
Notifications
You must be signed in to change notification settings - Fork 64
Delete Server in Tornjak Manager #676
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?
Delete Server in Tornjak Manager #676
Conversation
Signed-off-by: Harshit Gupta <[email protected]>
96c3be7 to
1f0bac4
Compare
Signed-off-by: Harshit Gupta <[email protected]>
9afe933 to
6aab04c
Compare
Signed-off-by: Harshit Gupta <[email protected]>
6aab04c to
aaa4f8f
Compare
Signed-off-by: Harshit Gupta <[email protected]>
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 PR implements server deletion functionality across the stack, removing the TODO comment about missing DELETE server capability. The implementation adds database, API, and frontend support for deleting servers.
Key changes:
- Added
DeleteServermethod to database interface and SQLite implementation - Implemented
/manager-api/server/delete/API endpoint with validation - Enabled frontend server deletion UI functionality with proper API integration
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/manager/db/db.go | Adds DeleteServer method to ManagerDB interface |
| pkg/manager/db/sqlite.go | Implements DeleteServer with row validation; removes TODO comment |
| pkg/manager/db/sqlite_test.go | Adds comprehensive test for server deletion scenarios |
| api/manager/api.go | Defines DeleteServerRequest struct for API payload |
| api/manager/server.go | Implements serverDelete HTTP handler with validation |
| frontend/src/components/tornjak-api-helpers.tsx | Adds serverDelete API client method |
| frontend/src/tables/servers-list-table.tsx | Enables delete functionality with IsManager check |
| docs/tornjak-ui-api-documentation.md | Documents new DELETE endpoint with example |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| serversListUpdateFunc(globalServersList.filter(el => | ||
| el.name !== inputData)) |
Copilot
AI
Nov 4, 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 filter logic is incorrect. It compares el.name (a string) with inputData (an object containing { server: { name: string } }). This should be el.name !== inputData.server.name to correctly filter out the deleted server.
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.
Works either/both ways. Should we stick to the current codebase pattern of checking el.name !== inputData?
| // let server: { name: string }[] = [], successMessage | ||
| deleteServer(selectedRows: readonly DenormalizedRow[]) { | ||
| if (!selectedRows || selectedRows.length === 0) return ""; | ||
| if (!IsManager) return ""; |
Copilot
AI
Nov 4, 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.
IsManager should be called as a function IsManager(), not checked as a value. Based on the codebase pattern in other files like agent-list.tsx, IsManager is used in conditional expressions like if (IsManager) {...}, suggesting it's a function or constant that should be invoked.
| if (!IsManager) return ""; | |
| if (!IsManager()) return ""; |
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.
Incorrect. agent-list.tsx also uses IsManager as a value, not as a function. The type of IsManager is a boolean, its not a function.
| successMessage = this.TornjakApi.serverDelete({ server: server[i] }, this.props.serversListUpdateFunc, this.props.globalServersList); | ||
| successMessage.then(function (result) { | ||
| if (result === "SUCCESS") { | ||
| window.alert(`SERVER "${server[i].name}" DELETED SUCCESSFULLY!`); | ||
| window.location.reload(); | ||
| } else { | ||
| window.alert(`Error deleting server "${server[i].name}": ` + result); |
Copilot
AI
Nov 4, 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.
Variable server[i] may have incorrect value in async callback. Since this is inside an async callback and i is captured by reference, by the time the callback executes, i will likely equal selectedRows.length. Consider storing server[i].name in a local constant before the async call or use forEach to properly capture the value.
| successMessage = this.TornjakApi.serverDelete({ server: server[i] }, this.props.serversListUpdateFunc, this.props.globalServersList); | |
| successMessage.then(function (result) { | |
| if (result === "SUCCESS") { | |
| window.alert(`SERVER "${server[i].name}" DELETED SUCCESSFULLY!`); | |
| window.location.reload(); | |
| } else { | |
| window.alert(`Error deleting server "${server[i].name}": ` + result); | |
| const serverName = server[i].name; | |
| successMessage = this.TornjakApi.serverDelete({ server: server[i] }, this.props.serversListUpdateFunc, this.props.globalServersList); | |
| successMessage.then(function (result) { | |
| if (result === "SUCCESS") { | |
| window.alert(`SERVER "${serverName}" DELETED SUCCESSFULLY!`); | |
| window.location.reload(); | |
| } else { | |
| window.alert(`Error deleting server "${serverName}": ` + result); |
Signed-off-by: Harshit Gupta <[email protected]>
Signed-off-by: Harshit Gupta <[email protected]>
Signed-off-by: Harshit Gupta <[email protected]>
Adds possibility to delete a server in Tornjak Manager. This will only delete the Server object from the Tornjak Manager database, it will not delete the Tornjak Server / Spire Server.
Closes #449