v0.1.0-alpha.11 #270
v0.1.0-alpha.11 #270
Conversation
test-cases: E2E test cases for Domain and Container flow
revert: e2e test cases
Feat e2e test
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 18077893 | Triggered | Generic Password | 772eb8b | scripts/setup.sh | View secret |
| 15933525 | Triggered | Generic Password | 75089c9 | docker-compose-test.yml | View secret |
| 18077894 | Triggered | Generic High Entropy Secret | 772eb8b | scripts/setup.sh | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
.gitignore (1)
15-15: Clarify scope of.vscodeignore pattern
.vscode(without a leading/) will match any.vscodedirectory at any depth.
If you only intend to exclude the root-level folder (the usual case) and make the directory intent explicit, consider:-.vscode +/.vscode/api/internal/routes.go (1)
247-248: Use the project logger instead of the std log for consistencyEverywhere else in this file we rely on the structured logger returned by
logger.NewLogger().
Falling back to the std-lib logger here breaks formatting / log-level consistency and bypasses any hooks you may have configured (e.g. JSON output, external sinks).- log.Printf("Server starting on port %s", PORT) - log.Printf("Swagger UI available at: http://localhost:%s/swagger/", PORT) + l.Infof("Server starting on port %s", PORT) + l.Infof("Swagger UI available at: http://localhost:%s/swagger/", PORT)docker-compose-staging.yml (1)
1-1: Minor: Consider maintaining consistent YAML formatting style.While both single and double quotes are valid for the version field, and both inline and multiline arrays are valid for the healthcheck test command, it's better to maintain consistency across all Docker Compose files in the project.
Also applies to: 48-49
docker-compose-test.yml (1)
4-15: Consider adding a healthcheck for the test database.The staging database includes a healthcheck configuration, but the test database doesn't. Adding a healthcheck would ensure the database is ready before dependent services start.
Add a healthcheck section after line 15:
networks: - nixopus-network + healthcheck: + test: + ["CMD-SHELL", "pg_isready -U ${TEST_DB_USERNAME:-nixopus} -d ${TEST_DB_NAME:-nixopus_test} -p 5432"] + interval: 5s + timeout: 5s + retries: 5api/internal/tests/feature-flags/feature_flags_test.go (1)
382-383: Remove extra spaces in comments.- description: " return false for disabled container feature", + description: "return false for disabled container feature",api/internal/tests/container/get_container_test.go (1)
138-138: Hardcoded container name assumption may cause test brittleness.The test specifically searches for
"nixopus-test-db-container"which assumes this container exists in the test environment. Consider documenting this dependency or making the test more flexible.Consider adding documentation about test environment requirements or making the test more robust:
-Store().Response().Body().JSON().JQ(`.data[] | select(.name == "nixopus-test-db-container") | .id`).In(&containerID), +Store().Response().Body().JSON().JQ(`.data[] | select(.name | test("test.*container")) | .id`).In(&containerID),api/internal/tests/container/get_container_logs_test.go (1)
21-24: Address the TODO comment about test container setup.The TODO comment indicates a need for proper test environment setup with container seeding. This is important for test reliability and should be addressed.
The TODO suggests implementing database seeding for E2E tests. This would improve test reliability by ensuring consistent test data.
Do you want me to help design a test setup script or open a new issue to track this improvement?
api/internal/tests/domain/domain_flow_test.go (5)
482-482: Fix typo in comment- domainID: domainID1, // Already deleted in first test case so expcected to throw 404 + domainID: domainID1, // Already deleted in first test case so expected to throw 404
692-692: Fix typo in comment- // assert agaisnst the updated domain name + // assert against the updated domain name
695-721: Remove redundant step prefixes in descriptionsThe "Step 5:" and "Step 6:" prefixes in the descriptions are redundant since the test flow is already clear from the context.
- Description("Step 5: Delete the domain"), + Description("Delete the domain"),- Description("Step 6: Verify domain is removed from domains listing"), + Description("Verify domain is removed from domains listing"),
737-737: Fix typo in comment- // Create a domain in the user' organization + // Create a domain in the user's organization
859-865: Use strings.Repeat for more efficient string generation- longDomainName := "" - for i := 0; i < 300; i++ { - longDomainName += "a" - } - longDomainName += ".nixopus.dev" + longDomainName := strings.Repeat("a", 300) + ".nixopus.dev"Don't forget to import the
stringspackage at the top of the file:import ( // ... other imports "strings" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.gitignore(1 hunks)api/api/versions.json(1 hunks)api/internal/features/domain/controller/create_domain.go(1 hunks)api/internal/features/domain/controller/delete_domain.go(1 hunks)api/internal/features/domain/controller/error_helpers.go(1 hunks)api/internal/features/domain/controller/get_domains.go(3 hunks)api/internal/features/domain/controller/update_domain.go(1 hunks)api/internal/features/domain/storage/init.go(5 hunks)api/internal/routes.go(1 hunks)api/internal/tests/container/get_container_logs_test.go(1 hunks)api/internal/tests/container/get_container_test.go(1 hunks)api/internal/tests/container/list_containers_test.go(1 hunks)api/internal/tests/domain/domain_flow_test.go(1 hunks)api/internal/tests/feature-flags/feature_flags_test.go(1 hunks)api/internal/tests/helper.go(1 hunks)docker-compose-staging.yml(2 hunks)docker-compose-test.yml(1 hunks)view/app/self-host/hooks/use_create_deployment.ts(0 hunks)view/app/self-host/hooks/use_update_deployment.ts(1 hunks)view/app/settings/domains/components/delete-domain.tsx(1 hunks)view/app/settings/domains/page.tsx(1 hunks)view/components/domains/random-subdomain-generator.tsx(1 hunks)view/redux/services/settings/domainsApi.ts(2 hunks)view/redux/types/domain.ts(1 hunks)
💤 Files with no reviewable changes (1)
- view/app/self-host/hooks/use_create_deployment.ts
🧰 Additional context used
🧬 Code Graph Analysis (9)
api/internal/features/domain/controller/update_domain.go (1)
api/internal/features/domain/types/domains.go (1)
ErrDomainNotFound(10-10)
api/internal/features/domain/controller/create_domain.go (1)
api/internal/features/domain/types/domains.go (1)
ErrDomainAlreadyExists(15-15)
api/internal/features/domain/controller/delete_domain.go (1)
api/internal/features/domain/types/domains.go (1)
ErrDomainNotFound(10-10)
api/internal/features/domain/storage/init.go (2)
api/internal/types/domain.go (1)
Domain(12-21)api/internal/features/domain/types/domains.go (1)
ErrDomainNotFound(10-10)
api/internal/features/domain/controller/get_domains.go (3)
api/internal/utils/get_user.go (2)
GetOrganizationID(37-54)GetUser(25-35)api/internal/features/logger/init.go (2)
Error(22-22)Info(20-20)api/internal/features/domain/types/domains.go (3)
ErrMissingID(24-24)ErrAccessDenied(26-26)ErrDomainNotFound(10-10)
api/internal/features/domain/controller/error_helpers.go (1)
api/internal/features/domain/types/domains.go (6)
ErrInvalidDomainID(13-13)ErrMissingDomainID(14-14)ErrDomainNameInvalid(27-27)ErrDomainNameTooLong(17-17)ErrDomainNameTooShort(18-18)ErrMissingDomainName(12-12)
api/internal/tests/container/list_containers_test.go (2)
api/internal/testutils/setup.go (1)
NewTestSetup(166-211)api/internal/tests/helper.go (1)
GetContainersURL(69-71)
api/internal/tests/container/get_container_test.go (3)
api/internal/testutils/setup.go (1)
NewTestSetup(166-211)api/internal/tests/helper.go (2)
GetContainersURL(69-71)GetContainerURL(73-75)api/internal/types/response.go (1)
Response(5-20)
view/redux/services/settings/domainsApi.ts (2)
view/redux/base-query.ts (1)
baseQueryWithReauth(57-90)view/redux/types/domain.ts (5)
Domain(1-6)CreateDomainRequest(13-16)UpdateDomainRequest(18-21)DeleteDomainRequest(23-25)RandomSubdomainResponse(8-11)
🔇 Additional comments (31)
api/internal/tests/helper.go (1)
69-99: LGTM!The new helper functions follow the established pattern and provide a clean abstraction for test URL construction.
api/internal/tests/feature-flags/feature_flags_test.go (1)
385-392: Verify the default behavior for non-existent and empty feature names.The tests expect that non-existent features and empty feature names return a default enabled state. This behavior might lead to unintended feature access.
Consider whether this is the intended behavior. It might be more secure to:
- Return 404 for non-existent features
- Return 400 for empty feature names
Also applies to: 418-425
api/api/versions.json (1)
6-6: LGTM!Version metadata update is consistent with the release timeline.
view/app/settings/domains/components/delete-domain.tsx (1)
29-29: LGTM! Improved type safety with structured request object.The change from passing a plain string
idto a structured object{ id }aligns with the API refactoring and improves type safety. This ensures consistency between the UI component and the Redux API service layer.view/app/self-host/hooks/use_update_deployment.ts (1)
45-45: LGTM! Aligns with API refactoring to fetch domains globally.The removal of the organization ID parameter from
useGetAllDomainsQuery()is consistent with the backend API changes where domains are now fetched globally without requiring an organization ID.view/app/settings/domains/page.tsx (1)
25-25: LGTM – please verify theactiveOrgguard
The removal of the organization ID parameter fromuseGetAllDomainsQuery()aligns with the new global domains API. However, the component still blocks rendering whenactiveOrg?.idis falsy:• File:
view/app/settings/domains/page.tsx
Lines ~27–31:if (!activeOrg?.id) { return ( <div className="container mx-auto py-6 space-y-8 max-w-4xl"> … </div> ); }Please confirm whether this guard is still required now that domains are fetched globally, or if it can be removed/updated to allow rendering without an active organization.
api/internal/features/domain/controller/delete_domain.go (1)
35-47: Excellent improvement in error handling with granular HTTP status codes.The enhanced error handling logic provides appropriate HTTP status codes based on error types:
- 400 Bad Request for invalid domain errors (validation issues)
- 404 Not Found for domain not found errors
- 500 Internal Server Error for unexpected errors
This follows REST API best practices and provides better error responses to clients, replacing the previous generic 500 error handling.
api/internal/features/domain/controller/create_domain.go (1)
46-58: Excellent improvement in error handling with appropriate HTTP status codes.The enhanced error handling logic provides granular HTTP status codes:
- 400 Bad Request for invalid domain errors (validation issues)
- 409 Conflict for domain already exists errors
- 500 Internal Server Error for unexpected errors
This improvement is consistent with the similar changes in
delete_domain.goand follows REST API best practices, providing meaningful error responses to clients.api/internal/features/domain/controller/update_domain.go (1)
45-57: LGTM: Enhanced error handling improves API response accuracy.The granular error handling correctly maps domain validation errors to HTTP 400 and domain not found errors to HTTP 404, providing more precise API responses compared to the previous generic 500 error handling.
view/redux/types/domain.ts (1)
8-25: LGTM: Well-structured TypeScript interfaces enhance type safety.The new interfaces provide strong typing for domain operations, improving code maintainability and preventing runtime type errors. The interface names and field structures are clear and consistent.
api/internal/features/domain/storage/init.go (2)
63-63: LGTM: Comprehensive soft deletion implementation.The consistent addition of
AND deleted_at IS NULLconditions across all query methods ensures that soft-deleted domains are properly excluded from results.Also applies to: 75-75, 81-81, 114-114, 125-125, 138-138, 150-150
89-107: LGTM: Well-implemented soft deletion with proper error handling.The soft deletion implementation correctly updates timestamps and validates affected rows to return
ErrDomainNotFoundwhen the domain doesn't exist or is already deleted.view/components/domains/random-subdomain-generator.tsx (1)
1-56: LGTM: Well-structured React component with proper error handling.The component follows React best practices with:
- Proper async error handling using try-catch
- Loading states with disabled button
- User feedback through toast notifications
- Conditional rendering for different states
- Optional callback prop for parent component integration
api/internal/features/domain/controller/error_helpers.go (2)
5-17: LGTM: Well-structured error classification helper.The
isInvalidDomainErrorfunction effectively centralizes domain validation error checking, improving code maintainability across controllers.
19-27: All referenced error constants are properly defined
I’ve confirmed that bothErrUserDoesNotBelongToOrganizationandErrPermissionDeniedare defined inapi/internal/features/domain/types/domains.go. No further action is required.api/internal/features/domain/controller/get_domains.go (2)
75-82: Excellent improvements to organization context handling and error responses.The enhanced organization ID extraction from context and improved error handling are well implemented. The change from 400 to 404 status code when no domains are available is semantically more correct. The same
isPermissionErrorfunction issue mentioned earlier applies here as well.Also applies to: 93-109, 118-122
45-50: Ignore missing import for isPermissionErrorThe helper
isPermissionErroris defined inapi/internal/features/domain/controller/error_helpers.gowithin the same package, so no import is required.
- Location of definition:
• api/internal/features/domain/controller/error_helpers.go:19–27No changes needed.
Likely an incorrect or invalid review comment.
view/redux/services/settings/domainsApi.ts (4)
4-10: Excellent type safety improvements.The addition of strongly typed interfaces for all request and response objects significantly improves type safety and prevents runtime errors.
17-26: Simplified query parameter handling aligns with backend changes.Removing the
organizationIdparameter fromgetAllDomainscorrectly reflects the backend enhancement to extract organization context internally from the request context.
27-37: Consistent use of typed request objects across all mutations.All mutation endpoints now use strongly typed request objects (
CreateDomainRequest,UpdateDomainRequest,DeleteDomainRequest), which improves maintainability and catches type errors at compile time.Also applies to: 38-48, 49-59
60-68: Proper response transformation for random subdomain generation.The explicit transformation to
RandomSubdomainResponseensures type safety and provides a clean interface for consuming components.api/internal/tests/container/get_container_test.go (1)
145-174: Comprehensive container structure validation is excellent.The detailed validation of container fields including ports, host configuration, and metadata provides thorough coverage and ensures API contract compliance.
api/internal/tests/container/list_containers_test.go (2)
21-63: Comprehensive test case coverage for authentication and authorization.The test cases effectively cover all authentication scenarios including valid tokens, missing tokens, invalid tokens, and malformed organization IDs. The test descriptions are clear and helpful for debugging.
127-155: Thorough error handling scenarios.The error handling tests cover important edge cases like malformed authorization headers and empty tokens, which helps ensure the API behaves correctly under various failure conditions.
api/internal/tests/container/get_container_logs_test.go (2)
150-227: Excellent coverage of log filtering parameters.The tests thoroughly cover all log filtering options including
tail,since,timestamps, andfollowparameters. This ensures the log API contract is properly validated.
343-396: Comprehensive permission testing across organizations.The cross-organization access tests are crucial for security validation. Testing both successful access within the organization and denied access across organizations ensures proper authorization enforcement.
api/internal/tests/domain/domain_flow_test.go (5)
1-10: LGTM!The imports are appropriate for the test file, and the package structure follows Go conventions.
12-129: LGTM!Comprehensive test coverage for domain creation with well-structured table-driven tests covering all edge cases including authorization, validation, and conflict scenarios.
131-227: LGTM!Well-structured test for fetching domains with proper setup and comprehensive scenarios including cross-organization access validation.
229-380: LGTM!Thorough test coverage for domain updates with proper setup and comprehensive validation of various error scenarios.
532-622: LGTM!Good test coverage for random subdomain generation with proper base domain setup.
Migration docs
docs: details on migrations
feat: Multiple Terminal Sessions
docs: details on migrations
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
view/app/terminal/terminal.tsx (1)
102-103: Good session initialization, consider persistence across page reloads.The session management is well-implemented with unique IDs. Consider persisting session state to localStorage to restore terminals across page reloads for better user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
view/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
api/internal/features/terminal/init.go(5 hunks)api/internal/realtime/init.go(3 hunks)api/internal/realtime/terminal_handler.go(2 hunks)view/app/terminal/terminal.tsx(6 hunks)view/app/terminal/utils/useTerminal.ts(8 hunks)view/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- view/package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/internal/realtime/init.go (2)
view/app/terminal/terminal.tsx (1)
Terminal(94-272)api/internal/features/terminal/init.go (1)
Terminal(29-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: review
🔇 Additional comments (3)
api/internal/realtime/init.go (1)
52-52: LGTM! Proper implementation of multi-terminal session support.The nested map structure correctly supports multiple terminal sessions per websocket connection, and the disconnect handler properly cleans up all associated sessions.
Also applies to: 76-76, 158-162
api/internal/features/terminal/init.go (1)
23-27: LGTM! Consistent terminal message format with session identification.The unified
TerminalMessagestruct withTerminalIdensures proper message routing in multi-session scenarios.Also applies to: 170-177, 206-211
api/internal/realtime/terminal_handler.go (1)
20-51: Excellent input validation and defensive programming.The implementation properly validates all required fields and safely initializes the nested map structure before creating new terminal sessions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
docs/migrations/templates.md (4)
14-18: Moveuuid-osspextension creation to a dedicated, one-off migration.Including
CREATE EXTENSION IF NOT EXISTS "uuid-ossp";in every “basic table” template will cause each new table migration to attempt the same extension creation.
While harmless in dev, this bloats prod migration history and may be blocked by RDS‐style managed Postgres where extension creation is restricted.Diff suggestion for this template block:
- CREATE EXTENSION IF NOT EXISTS "uuid-ossp";Add a separate “000_enable_uuid_ossp_up.sql / …_down.sql” pair instead.
53-60: Prefer explicit index names over templated[table_name]_[parent_table]_id.Generated names can exceed the 63-char Postgres limit for long table names, silently truncating and risking duplicates.
Consider documenting a max-length check or a hash-suffix scheme.
227-243: Point out that data-migration rollbacks may be lossy.The “Down” template assumes original values are known and rows weren’t touched after the “Up”.
Add a warning that down migrations for data moves are best-effort and may require full backups instead.
268-279: Minor grammar tweak for clarity.Insert missing article “the”.
-3. **Update the file header** with proper version and description +3. **Update the file header** with the proper version and descriptiondocs/migrations/quick-reference.md (3)
55-57: Create index after large column adds with CONCURRENTLY.For big tables this blocking
CREATE INDEXmay lock writes. DocumentCREATE INDEX CONCURRENTLYoption for production safety.
147-151: Example rollback lacks error handling.Consider demonstrating error check on
MigrateDown()similar to the “Manual Migration Testing” section to maintain consistency.err := migrator.MigrateDown() if err != nil { log.Fatal(err) }
157-165: Add language identifier to fenced block (markdownlint MD040).-``` +```text {sequence_no}_{description}_{direction}.sql
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
assets/db-schema.pngis excluded by!**/*.png
📒 Files selected for processing (5)
api/api/versions.json(1 hunks)docs/.vitepress/config.mts(1 hunks)docs/migrations/index.md(1 hunks)docs/migrations/quick-reference.md(1 hunks)docs/migrations/templates.md(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- docs/.vitepress/config.mts
- api/api/versions.json
- docs/migrations/index.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/migrations/quick-reference.md
157-157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 LanguageTool
docs/migrations/templates.md
[uncategorized] ~277-~277: You might be missing the article “the” here.
Context: ...oes 3. Update the file header with proper version and description 4. **Test the m...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~291-~291: Loose punctuation mark.
Context: ...le_down.sql ### Table Names -users, user_preferences, user_sessions-...
(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~291-~291: Two consecutive commas
Context: ...e Names - users, user_preferences, user_sessions - organizations, organization_members, `organization_s...
(DOUBLE_PUNCTUATION)
[typographical] ~292-~292: Two consecutive commas
Context: ...organizations, organization_members, organization_settings-applications, application_configs, application_dep...
(DOUBLE_PUNCTUATION)
[uncategorized] ~297-~297: Loose punctuation mark.
Context: ...s ### Index Names -idx_users_email, idx_users_created_at-idx_user_pref...
(UNLIKELY_OPENING_PUNCTUATION)
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores
Documentation