fix all open github issues#11
Conversation
- Add session_timeout_minutes global setting with 5 minute default - Replace static SESSION_IDLE_TIMEOUT with dynamic RGW_SETTING_SESSION_TIMEOUT_MINUTES - Add SessionTTLProvider interface to session manager for dynamic TTL fetching - Add StaticTTLProvider for test convenience - Add /api/v1/settings/session-configuration endpoint for session config - Add SessionTimeoutCard component to admin settings page - Refactor settings/service.go into focused files to stay under 500 line limit: - service_global.go: global settings methods - service_app.go: app settings methods - service_mfa.go: MFA settings methods - service_deploy.go: deploy settings methods - service_session.go: session timeout methods - service_env_helpers.go: env var helper methods - Update tests to use StaticTTLProvider 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add `deploy-approval list` and `deploy-approval get` subcommands to the CLI to enable querying deploy approval requests: - `list`: Lists deploy approval requests with filters (status, open-only, limit) - `get`: Retrieves details for a specific deploy approval request by UUID Includes UUID validation on the get command to prevent path traversal attacks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…alue Addresses coderabbit suggestion to add observability when the TTL provider returns an error or non-positive value, helping operators detect misconfigurations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Relocated the ActionButtons component below RequestDetailsCard in the deploy approval request detail page. This improves UX by allowing users to review the request details before taking action. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Addresses GitHub issue #2: Commands with -a/--app flag now consistently resolve app names from .convox/app file or directory name when not explicitly provided. Changes: - Add resolveAppFlag() helper in common.go to centralize app resolution - Add appFlagHelp constant for consistent help text - Update all commands with -a flag to call resolveAppFlag() before delegating to Convox SDK - Affected commands: builds, apps info, deploy, releases, env, logs, ps, exec, run, restart, scale, resources This ensures the CLI works correctly from any directory with a .convox/app file, matching the behavior users expect from the original convox CLI. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Address coderabbit review suggestion: the three SetGlobalSetting calls in SetMFASettings were executed sequentially without a transaction. If one failed, earlier settings would remain persisted, leaving MFA configuration in a partially updated state. Changes: - Add UpsertSettingsInTx to db/settings.go for atomic batch updates - Add SetGlobalSettingsInTx helper to settings service - Update SetMFASettings to use transactional batch update - All three MFA settings now succeed or fail together 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…MFA flow Fixes GitHub issue #10: MFA modal was getting stuck after verification when creating API tokens because handleStepUpError didn't provide onResolve/onReject callbacks, so the result was never captured. Changes: - Update create-token-dialog to use runWithMFAGuard instead of handleStepUpError - Update edit-token-dialog to use runWithMFAGuard - Update delete-token-dialog to use runWithMFAGuard - Add guards for MFA cancellation (check for undefined response) - Remove unused handleStepUpError from use-token-mutations - Update test mock to include runWithMFAGuard The runWithMFAGuard pattern properly wraps the async operation and returns a Promise that resolves with the result when MFA verification succeeds, allowing the calling code to handle the response correctly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…tons Fixes GitHub issue #8: When clicking the approve tick on the deploy approvals table, all tick buttons were changing to spinners instead of just the one that was clicked. Changes: - Add approvingId state to track which specific request is being approved - Pass approvingId and rejectingId to row components instead of global pending - Compare request ID with tracking ID to determine which button shows spinner - Clear tracking ID on mutation success/error 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
… command - Add build-time version injection via Vite (package.json version + git commit hash) - Display version in web UI sidebar footer - Update backend /api/v1/info endpoint to include version info - Add ldflags to Go build for version injection - Add `rack-gateway version` command showing client, gateway, and server versions - Add `rack-gateway gateway` command showing gateway server info Also includes: - Fix API token edit dialog not closing after save (mutation wasn't returning response) - Fix deploy approval reject loading indicator (was using dialog state instead of mutation state) - Eliminate code duplication in GetApprovedDeployCommands (use extractStringSlice helper) - Add test assertion for edit dialog close after rename 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When GetAppSetting falls back to a global default for ci_provider or vcs_provider, it was passing "" as the default to GetGlobalSetting. This caused it to return "" instead of the hardcoded default from DefaultGlobalSettings when no global setting was in the database. On EU/US racks, default_ci_provider was not in the database, so ci_provider returned "" instead of "circleci", causing CircleCI auto job approval to silently fail. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThis PR embeds version metadata (from package.json and git commit hash) into the gateway binary at build time, comprehensively refactors settings management to use a TTL provider interface for session configuration, adds new CLI commands for displaying gateway information and managing deploy approval requests with multi-rack search support, and extends the web UI with session timeout configuration and improved deploy approval workflows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Here's the code health analysis summary for commits Analysis Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
docs/CONFIGURATION.md (1)
28-29: Consider minor grammar improvement.The documentation update correctly reflects the new session timeout configuration mechanism. However, the sentence on line 29 could be slightly improved for clarity.
Consider revising for better grammar:
-- `RGW_SETTING_SESSION_TIMEOUT_MINUTES` (default: `5`) - - Sliding inactivity window for browser sessions in minutes. Can also be configured via the Settings page in the admin UI (database value takes precedence). + - Sliding inactivity window for browser sessions in minutes. This can also be configured via the Settings page in the admin UI (database value takes precedence).web/src/pages/settings-page.tsx (1)
63-65: Consider UI layout consistency.The
SessionTimeoutCardis wrapped in its own two-column grid, creating a standalone row. This differs from the pattern on lines 59-62 whereMfaConfigCardandDestructiveActionsCardshare a row.If no second card is planned for this row, consider placing it outside the grid or using a single-column layout for clarity:
- <div className="grid gap-6 md:grid-cols-2"> - <SessionTimeoutCard disabled={!isAdmin} settings={globalSettings} /> - </div> + <SessionTimeoutCard disabled={!isAdmin} settings={globalSettings} />Or, if a second card might be added later, this structure is fine. Otherwise, the current implementation works correctly and the card integrates properly with admin gating and settings data.
internal/cli/deploy_approvals_get.go (1)
13-57: Deploy approval “get” command is solid; consider stricter output handlingThe command flow (rack resolution, UUID validation, gatewayRequest, and dual JSON/human output) is clean and consistent with existing patterns. One optional improvement would be to treat non-empty, non-
jsonvalues for--outputas invalid instead of silently falling back to the human format, e.g. by switching onoutputand returning an error for unknown formats.Also applies to: 59-93
internal/cli/auth.go (1)
130-157: GetGatewayInfo implementation looks correct; optional error‑formatting tweakThe helper correctly builds the URL, sends an authenticated request via
HTTPClient, validatesStatusOK, and decodes intoGatewayInfoResponse; resource handling is safe.If you want error messages consistent with other auth flows, you could optionally reuse
RenderGatewayErrorwhenStatusCode != http.StatusOKinstead of returning the raw body string. Not required for correctness.internal/cli/convox_env.go (1)
25-25: Env command app handling is improved; consider centralizing resolution laterUsing
appFlagHelpandresolveAppFlagforenv edit/set/unsetaligns these flows with other CLI commands and looks good.As a follow‑up, you might consider reusing
resolveAppFlag(or a shared helper under the hood) inenvGetGateway/envListGatewayas well, so all env commands rely on a single app resolution path and can’t drift over time.Also applies to: 42-44, 58-58, 74-74, 86-88, 102-102, 116-118, 132-132
internal/gateway/handlers/auth_test.go (1)
91-91: SessionManager TTLProvider wiring in tests is correctUpdating
NewSessionManagercall sites to use&auth.StaticTTLProvider{TTL: time.Hour}is consistent with the new constructor and keeps test behavior the same as the previous fixed one‑hour TTL. If you find yourself adding more tests like this, a small helper (e.g.,newTestSessionManager(t, db *db.Database)) could reduce repetition, but not required here.Also applies to: 152-152, 198-198, 231-231, 266-266, 304-304, 393-393, 410-410
internal/gateway/settings/service_session.go (1)
6-21: Consider error handling pattern for graceful degradation.Both methods return a default value (5 minutes) even when an error occurs. This pattern provides graceful degradation but may mask configuration issues. If the intent is to always provide a valid timeout (even on error), consider documenting this behavior in the function comments to clarify that callers can rely on the returned value regardless of error.
Alternatively, if errors should halt execution, follow the standard Go pattern of returning zero values with the error.
internal/gateway/settings/service_mfa.go (1)
57-77: Consider avoiding mutation of input parameter.Lines 64 and 67 modify the input
settingsparameter by setting default values. While this works, it's generally better practice to either:
- Return a new
*db.MFASettingswith corrected values, or- Document this side effect clearly in the function comment
This makes the function's behavior more explicit and avoids surprising callers.
Example:
-// SetMFASettings stores MFA configuration in the database atomically. +// SetMFASettings stores MFA configuration in the database atomically. +// Non-positive TTL and step-up window values are replaced with defaults (30 days and 10 minutes). // All settings are updated within a single transaction to prevent partial updates. func (s *Service) SetMFASettings(settings *db.MFASettings, updatedByUserID *int64) error {internal/cli/deploy_approvals_list.go (1)
92-121: Consider simplifying the return type.The function always returns
niland never produces an error. You could simplify by removing the error return, or keep it for API consistency with other print functions.-func printDeployApprovalTable(requests []deployApprovalRequest) error { +func printDeployApprovalTable(requests []deployApprovalRequest) { fmt.Printf("%-36s %-10s %-20s %-40s %s\n", "ID", "STATUS", "CREATED", "MESSAGE", "TOKEN") fmt.Println(strings.Repeat("-", 130)) for _, req := range requests { // ... existing logic ... } - - return nil }If kept as-is for consistency, this is acceptable.
internal/gateway/settings/service_global.go (1)
29-42: Consider adding audit logging for settings modifications.The methods
SetGlobalSetting,DeleteGlobalSetting, andSetGlobalSettingsInTxmodify global configuration without audit logging. Based on coding guidelines forinternal/gateway/**/*.go, sensitive data operations should use the audit logger.Consider adding audit log entries when settings are created, updated, or deleted, capturing the user ID and the affected setting keys.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/gateway/openapi/generated/swagger.jsonis excluded by!**/generated/**
📒 Files selected for processing (66)
Dockerfile(1 hunks)docs/CONFIGURATION.md(1 hunks)internal/cli/auth.go(1 hunks)internal/cli/cli_gateway.go(1 hunks)internal/cli/cli_version.go(1 hunks)internal/cli/common.go(1 hunks)internal/cli/convox_apps.go(2 hunks)internal/cli/convox_builds.go(11 hunks)internal/cli/convox_deploy.go(10 hunks)internal/cli/convox_env.go(8 hunks)internal/cli/convox_logs.go(2 hunks)internal/cli/convox_processes.go(14 hunks)internal/cli/convox_resources.go(8 hunks)internal/cli/deploy_approvals_command.go(1 hunks)internal/cli/deploy_approvals_get.go(1 hunks)internal/cli/deploy_approvals_list.go(1 hunks)internal/cli/root.go(1 hunks)internal/cli/types.go(1 hunks)internal/gateway/app/setup.go(2 hunks)internal/gateway/auth/cli_only_test.go(1 hunks)internal/gateway/auth/service_test.go(2 hunks)internal/gateway/auth/session_manager.go(5 hunks)internal/gateway/config/config.go(0 hunks)internal/gateway/db/settings.go(1 hunks)internal/gateway/handlers/api_handler_info.go(2 hunks)internal/gateway/handlers/auth_mfa_verification_test.go(2 hunks)internal/gateway/handlers/auth_test.go(8 hunks)internal/gateway/handlers/deploy_approval_requests_mfa_test.go(1 hunks)internal/gateway/handlers/dto.go(1 hunks)internal/gateway/handlers/settings.go(2 hunks)internal/gateway/handlers/settings_test.go(1 hunks)internal/gateway/middleware/auth_test.go(1 hunks)internal/gateway/middleware/mfa_stepup_totp_test.go(1 hunks)internal/gateway/routes/route_registration.go(1 hunks)internal/gateway/settings/defaults.go(4 hunks)internal/gateway/settings/groups.go(2 hunks)internal/gateway/settings/service.go(0 hunks)internal/gateway/settings/service_app.go(1 hunks)internal/gateway/settings/service_deploy.go(1 hunks)internal/gateway/settings/service_env_helpers.go(1 hunks)internal/gateway/settings/service_global.go(1 hunks)internal/gateway/settings/service_mfa.go(1 hunks)internal/gateway/settings/service_session.go(1 hunks)internal/gateway/settings/service_test.go(1 hunks)internal/gateway/version/version.go(1 hunks)taskfiles/Taskfile.go.yml(1 hunks)web/biome.json(1 hunks)web/e2e/manage.spec.ts(1 hunks)web/src/api/openapi.json(3 hunks)web/src/api/schemas/handlersInfoResponse.ts(1 hunks)web/src/api/schemas/handlersVersionInfo.ts(1 hunks)web/src/api/schemas/index.ts(1 hunks)web/src/api/types.generated.ts(2 hunks)web/src/components/layout.tsx(1 hunks)web/src/pages/deploy-approval-request-detail-page.tsx(1 hunks)web/src/pages/deploy-approval-requests-page.tsx(10 hunks)web/src/pages/settings-page.tsx(2 hunks)web/src/pages/settings/session-timeout-card.tsx(1 hunks)web/src/pages/tokens-page.test.tsx(1 hunks)web/src/pages/tokens-page/create-token-dialog.tsx(3 hunks)web/src/pages/tokens-page/delete-token-dialog.tsx(3 hunks)web/src/pages/tokens-page/edit-token-dialog.tsx(3 hunks)web/src/pages/tokens-page/use-token-mutations.ts(1 hunks)web/src/vite-env.d.ts(1 hunks)web/tsconfig.tests.json(1 hunks)web/vite.config.ts(2 hunks)
💤 Files with no reviewable changes (2)
- internal/gateway/config/config.go
- internal/gateway/settings/service.go
🧰 Additional context used
📓 Path-based instructions (17)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Maximum 500 lines per Go code file - refactor immediately if approaching this limit or split into smaller, focused modules
Maximum 100 lines per function (Go) - enforced by golangci-lint using funlen linter
Maximum 50 statements per function (Go) - enforced by golangci-lint
Maximum cognitive complexity of 15 (Go) - enforced by golangci-lint using gocognit linter
Line length maximum 120 characters - enforced by golangci-lint lll rule. Exception: For Go struct tag lines that cannot be broken without invalidating formatting (where gofmt/goimports always rejoin them and field+tag exceeds 120 chars), add//nolint:lllat end of line only
No//nolintcomments allowed except for struct tag line length exceptions - fix the code instead of suppressing warnings
Forbidden patterns in Go:panic(use error returns),fmt.Print*except in CLI output (use proper logging),time.Sleep(use context with timeout)
Zero code duplication allowed in production code - threshold 10 lines or 50 tokens, enforced by jscpd
Usegovulncheckto check for known Go vulnerabilities - required security scan
Files:
internal/cli/root.gointernal/gateway/auth/cli_only_test.gointernal/cli/convox_apps.gointernal/cli/common.gointernal/cli/convox_env.gointernal/gateway/settings/service_mfa.gointernal/cli/cli_gateway.gointernal/gateway/db/settings.gointernal/gateway/settings/service_test.gointernal/gateway/handlers/api_handler_info.gointernal/cli/types.gointernal/gateway/handlers/deploy_approval_requests_mfa_test.gointernal/gateway/settings/service_deploy.gointernal/gateway/routes/route_registration.gointernal/gateway/settings/defaults.gointernal/cli/deploy_approvals_list.gointernal/cli/deploy_approvals_get.gointernal/cli/cli_version.gointernal/gateway/auth/service_test.gointernal/cli/convox_resources.gointernal/gateway/settings/service_global.gointernal/gateway/settings/service_app.gointernal/gateway/settings/groups.gointernal/gateway/version/version.gointernal/cli/auth.gointernal/gateway/auth/session_manager.gointernal/gateway/settings/service_env_helpers.gointernal/gateway/settings/service_session.gointernal/gateway/handlers/settings.gointernal/gateway/handlers/auth_test.gointernal/gateway/handlers/auth_mfa_verification_test.gointernal/cli/convox_processes.gointernal/cli/convox_logs.gointernal/cli/deploy_approvals_command.gointernal/gateway/middleware/auth_test.gointernal/cli/convox_builds.gointernal/gateway/handlers/dto.gointernal/cli/convox_deploy.gointernal/gateway/handlers/settings_test.gointernal/gateway/middleware/mfa_stepup_totp_test.gointernal/gateway/app/setup.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Maximum 1000 lines per Go test file - refactor immediately if approaching this limit or split tests into multiple files
Files:
internal/gateway/auth/cli_only_test.gointernal/gateway/settings/service_test.gointernal/gateway/handlers/deploy_approval_requests_mfa_test.gointernal/gateway/auth/service_test.gointernal/gateway/handlers/auth_test.gointernal/gateway/handlers/auth_mfa_verification_test.gointernal/gateway/middleware/auth_test.gointernal/gateway/handlers/settings_test.gointernal/gateway/middleware/mfa_stepup_totp_test.go
internal/gateway/auth/**/*.go
📄 CodeRabbit inference engine (internal/gateway/CLAUDE.md)
Validate requests against configured Google Workspace domain in authentication handlers
Files:
internal/gateway/auth/cli_only_test.gointernal/gateway/auth/service_test.gointernal/gateway/auth/session_manager.go
internal/gateway/**/*.go
📄 CodeRabbit inference engine (internal/gateway/CLAUDE.md)
Use audit logger for all sensitive data in Go files
Files:
internal/gateway/auth/cli_only_test.gointernal/gateway/settings/service_mfa.gointernal/gateway/db/settings.gointernal/gateway/settings/service_test.gointernal/gateway/handlers/api_handler_info.gointernal/gateway/handlers/deploy_approval_requests_mfa_test.gointernal/gateway/settings/service_deploy.gointernal/gateway/routes/route_registration.gointernal/gateway/settings/defaults.gointernal/gateway/auth/service_test.gointernal/gateway/settings/service_global.gointernal/gateway/settings/service_app.gointernal/gateway/settings/groups.gointernal/gateway/version/version.gointernal/gateway/auth/session_manager.gointernal/gateway/settings/service_env_helpers.gointernal/gateway/settings/service_session.gointernal/gateway/handlers/settings.gointernal/gateway/handlers/auth_test.gointernal/gateway/handlers/auth_mfa_verification_test.gointernal/gateway/middleware/auth_test.gointernal/gateway/handlers/dto.gointernal/gateway/handlers/settings_test.gointernal/gateway/middleware/mfa_stepup_totp_test.gointernal/gateway/app/setup.go
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Maximum 500 lines per TypeScript/TSX code file - refactor immediately if approaching this limit or split into smaller, focused modules
Use TypeScript strict mode and avoid using
anytype
Files:
web/src/api/schemas/handlersInfoResponse.tsweb/src/pages/deploy-approval-request-detail-page.tsxweb/e2e/manage.spec.tsweb/src/api/schemas/handlersVersionInfo.tsweb/src/pages/settings/session-timeout-card.tsxweb/vite.config.tsweb/src/pages/deploy-approval-requests-page.tsxweb/src/api/types.generated.tsweb/src/pages/tokens-page/create-token-dialog.tsxweb/src/api/schemas/index.tsweb/src/pages/tokens-page.test.tsxweb/src/pages/settings-page.tsxweb/src/pages/tokens-page/delete-token-dialog.tsxweb/src/pages/tokens-page/use-token-mutations.tsweb/src/vite-env.d.tsweb/src/components/layout.tsxweb/src/pages/tokens-page/edit-token-dialog.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: No// biome-ignorecomments allowed - fix the code, don't suppress the linter. Zero tolerance enforcement.
Zero code duplication allowed in production code - threshold 10 lines or 50 tokens, enforced by jscpd
Files:
web/src/api/schemas/handlersInfoResponse.tsweb/src/pages/deploy-approval-request-detail-page.tsxweb/e2e/manage.spec.tsweb/src/api/schemas/handlersVersionInfo.tsweb/src/pages/settings/session-timeout-card.tsxweb/vite.config.tsweb/src/pages/deploy-approval-requests-page.tsxweb/src/api/types.generated.tsweb/src/pages/tokens-page/create-token-dialog.tsxweb/src/api/schemas/index.tsweb/src/pages/tokens-page.test.tsxweb/src/pages/settings-page.tsxweb/src/pages/tokens-page/delete-token-dialog.tsxweb/src/pages/tokens-page/use-token-mutations.tsweb/src/vite-env.d.tsweb/src/components/layout.tsxweb/src/pages/tokens-page/edit-token-dialog.tsx
web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Always import toast from
@/components/ui/use-toast, never directly fromreact-hot-toast
Files:
web/src/api/schemas/handlersInfoResponse.tsweb/src/pages/deploy-approval-request-detail-page.tsxweb/e2e/manage.spec.tsweb/src/api/schemas/handlersVersionInfo.tsweb/src/pages/settings/session-timeout-card.tsxweb/vite.config.tsweb/src/pages/deploy-approval-requests-page.tsxweb/src/api/types.generated.tsweb/src/pages/tokens-page/create-token-dialog.tsxweb/src/api/schemas/index.tsweb/src/pages/tokens-page.test.tsxweb/src/pages/settings-page.tsxweb/src/pages/tokens-page/delete-token-dialog.tsxweb/src/pages/tokens-page/use-token-mutations.tsweb/src/vite-env.d.tsweb/src/components/layout.tsxweb/src/pages/tokens-page/edit-token-dialog.tsx
web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (web/.cursor/rules/ultracite.mdc)
web/**/*.{ts,tsx,js,jsx}: Don't useaccessKeyattribute on any HTML element
Don't setaria-hidden="true"on focusable elements
Don't add ARIA roles, states, and properties to elements that don't support them
Don't use distracting elements like<marquee>or<blink>
Only use thescopeprop on<th>elements
Don't assign non-interactive ARIA roles to interactive HTML elements
Make sure label elements have text content and are associated with an input
Don't assign interactive ARIA roles to non-interactive HTML elements
Don't assigntabIndexto non-interactive HTML elements
Don't use positive integers fortabIndexproperty
Don't include "image", "picture", or "photo" in img alt prop
Don't use explicit role property that's the same as the implicit/default role
Make static elements with click handlers use a valid role attribute
Always include atitleelement for SVG elements
Give all elements requiring alt text meaningful information for screen readers
Make sure anchors have content that's accessible to screen readers
AssigntabIndexto non-interactive HTML elements witharia-activedescendant
Include all required ARIA attributes for elements with ARIA roles
Make sure ARIA properties are valid for the element's supported roles
Always include atypeattribute for button elements
Make elements with interactive roles and handlers focusable
Give heading elements content that's accessible to screen readers (not hidden witharia-hidden)
Always include alangattribute on the html element
Always include atitleattribute for iframe elements
AccompanyonClickwith at least one of:onKeyUp,onKeyDown, oronKeyPress
AccompanyonMouseOver/onMouseOutwithonFocus/onBlur
Include caption tracks for audio and video elements
Use semantic elements instead of role attributes in JSX
Make sure all anchors are valid and navigable
Ensure all ARIA properties (aria-*) are valid
Use valid, non-abstract ARIA roles for elements with ARIA roles
Use vali...
Files:
web/src/api/schemas/handlersInfoResponse.tsweb/src/pages/deploy-approval-request-detail-page.tsxweb/e2e/manage.spec.tsweb/src/api/schemas/handlersVersionInfo.tsweb/src/pages/settings/session-timeout-card.tsxweb/vite.config.tsweb/src/pages/deploy-approval-requests-page.tsxweb/src/api/types.generated.tsweb/src/pages/tokens-page/create-token-dialog.tsxweb/src/api/schemas/index.tsweb/src/pages/tokens-page.test.tsxweb/src/pages/settings-page.tsxweb/src/pages/tokens-page/delete-token-dialog.tsxweb/src/pages/tokens-page/use-token-mutations.tsweb/src/vite-env.d.tsweb/src/components/layout.tsxweb/src/pages/tokens-page/edit-token-dialog.tsx
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx}: Use clear and descriptive variable names that convey intent
Write modular code by breaking down large functions into smaller, reusable functions
Add comments to explain complex logic and edge cases
Use proper error handling with try-catch blocks and meaningful error messages
Prefer functional programming patterns over imperative code where applicable
Keep functions small and focused on a single responsibility
Avoid deep nesting and use early returns to improve readability
Files:
web/src/api/schemas/handlersInfoResponse.tsweb/src/pages/deploy-approval-request-detail-page.tsxweb/e2e/manage.spec.tsweb/src/api/schemas/handlersVersionInfo.tsweb/src/pages/settings/session-timeout-card.tsxweb/vite.config.tsweb/src/pages/deploy-approval-requests-page.tsxweb/src/api/types.generated.tsweb/src/pages/tokens-page/create-token-dialog.tsxweb/src/api/schemas/index.tsweb/src/pages/tokens-page.test.tsxweb/src/pages/settings-page.tsxweb/src/pages/tokens-page/delete-token-dialog.tsxweb/src/pages/tokens-page/use-token-mutations.tsweb/src/vite-env.d.tsweb/src/components/layout.tsxweb/src/pages/tokens-page/edit-token-dialog.tsx
web/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
For fast feedback when changing ONLY E2E test files, run
cd web && bunx playwright testdirectly instead oftask web:e2e. Only use this when test files are the ONLY changes.
Files:
web/e2e/manage.spec.ts
web/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/**/*.{test,spec}.{ts,tsx}: Use relative paths (not path aliases like@/) invi.mock()calls - Vitest treats aliased and relative paths as different module instances
When mocking React Query queries that child components read from cache without aqueryFn, addenabled: falseto prevent React Query from attempting to fetch
Suppress React Query console errors about missingqueryFnin tests usingvi.spyOn(console, 'error').mockImplementation()to filter 'No queryFn was passed' messages
UsemockImplementationwith URL routing instead of sequentialmockResolvedValueOncefor mocking parallel React Query requests
Test router basepath handling for/app, including/loginand/auth/callbackroutes in Vitest unit tests
Test auth flows, API adapters (with mocked network, not depending on browser), and critical UI/behavior for Users, Tokens, and Audit pages in unit tests
Files:
web/e2e/manage.spec.tsweb/src/pages/tokens-page.test.tsx
web/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (web/.cursor/rules/ultracite.mdc)
web/**/*.{test,spec}.{ts,tsx,js,jsx}: Don't nest describe() blocks too deeply in test files
Don't use callbacks in asynchronous tests and hooks
Don't have duplicate hooks in describe blocks
Don't use export or module.exports in test files
Don't use focused tests
Make sure the assertion function, like expect, is placed inside an it() function call
Don't use disabled tests
Files:
web/e2e/manage.spec.tsweb/src/pages/tokens-page.test.tsx
internal/gateway/db/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Database layer code should follow Repository pattern for data access abstractions
Files:
internal/gateway/db/settings.go
web/vite.config.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
process.env.GATEWAY_PORTfor proxy configuration instead of hardcoding ports
Files:
web/vite.config.ts
internal/gateway/auth/session_manager.go
📄 CodeRabbit inference engine (internal/gateway/CLAUDE.md)
Implement session creation and validation in
auth/session_manager.gowith opaque tokens stored in database
Files:
internal/gateway/auth/session_manager.go
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Maximum 1000 lines per TypeScript test file - refactor immediately if approaching this limit or split tests into multiple files
Files:
web/src/pages/tokens-page.test.tsx
**/*.test.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests for all critical functionality
Files:
web/src/pages/tokens-page.test.tsx
🧬 Code graph analysis (38)
internal/cli/root.go (1)
internal/cli/cli_gateway.go (1)
GatewayCommand(11-19)
internal/gateway/auth/cli_only_test.go (1)
internal/gateway/auth/session_manager.go (2)
NewSessionManager(67-73)StaticTTLProvider(31-33)
web/src/api/schemas/handlersInfoResponse.ts (1)
web/src/api/schemas/handlersVersionInfo.ts (1)
HandlersVersionInfo(9-12)
internal/cli/common.go (2)
internal/convox/commands.go (1)
Command(13-18)internal/cli/utils.go (1)
ResolveApp(78-86)
internal/gateway/settings/service_mfa.go (3)
internal/gateway/settings/service.go (1)
Service(16-18)internal/gateway/db/settings.go (2)
MFASettings(239-243)SettingUpdate(135-138)internal/gateway/settings/defaults.go (3)
KeyMFARequireAllUsers(28-28)KeyTrustedDeviceTTLDays(32-32)KeyStepUpWindowMinutes(31-31)
web/src/pages/deploy-approval-request-detail-page.tsx (1)
web/src/components/deploy-approval-request/details.tsx (3)
RequestHeader(45-89)RequestDetailsCard(153-172)ActionButtons(101-147)
internal/cli/cli_gateway.go (5)
internal/cli/utils.go (1)
SilenceOnError(17-26)internal/cli/rack.go (1)
SelectedRack(11-32)internal/cli/config.go (1)
LoadRackAuth(149-175)internal/cli/auth.go (1)
GetGatewayInfo(131-157)internal/cli/types.go (2)
GatewayInfoResponse(103-108)GatewayIntegrationsInfo(125-129)
web/e2e/manage.spec.ts (1)
web/e2e/fixtures.ts (1)
expect(311-311)
internal/gateway/db/settings.go (1)
internal/gateway/db/database.go (1)
Database(21-26)
internal/gateway/settings/service_test.go (3)
internal/gateway/testutil/dbtest/dbtest.go (1)
NewDatabase(50-65)internal/gateway/settings/service.go (2)
NewService(21-23)SourceGlobalDefault(33-33)internal/gateway/settings/defaults.go (2)
KeyCIProvider(96-96)KeyVCSProvider(107-107)
internal/gateway/handlers/api_handler_info.go (3)
internal/gateway/handlers/dto.go (2)
VersionInfo(182-185)InfoResponse(188-193)internal/gateway/version/version.go (2)
Version(6-6)CommitHash(9-9)web/src/lib/api.ts (1)
InfoResponse(84-84)
internal/gateway/handlers/deploy_approval_requests_mfa_test.go (1)
internal/gateway/auth/session_manager.go (2)
NewSessionManager(67-73)StaticTTLProvider(31-33)
internal/gateway/settings/service_deploy.go (2)
internal/gateway/settings/service.go (1)
Service(16-18)internal/gateway/settings/defaults.go (3)
KeyAllowDestructiveActions(23-23)KeyDeployApprovalsEnabled(26-26)KeyDeployApprovalWindowMinutes(27-27)
internal/cli/deploy_approvals_get.go (1)
internal/convox/commands.go (1)
Command(13-18)
internal/cli/cli_version.go (1)
internal/cli/globals.go (1)
Version(25-25)
internal/gateway/auth/service_test.go (1)
internal/gateway/auth/session_manager.go (2)
NewSessionManager(67-73)StaticTTLProvider(31-33)
web/src/pages/settings/session-timeout-card.tsx (6)
web/src/pages/settings/types.ts (1)
GlobalSettingsResponse(3-3)web/src/contexts/step-up-context.tsx (1)
useStepUp(353-359)web/src/hooks/use-mutation.ts (1)
useMutation(65-88)web/src/lib/api.ts (1)
api(428-460)web/src/api/schemas/settingsSetting.ts (1)
SettingsSetting(10-15)web/src/lib/error-utils.ts (1)
toastAPIError(14-19)
internal/gateway/settings/service_global.go (3)
internal/gateway/settings/service.go (2)
Service(16-18)Setting(37-41)internal/gateway/settings/defaults.go (1)
DefaultGlobalSettings(160-170)internal/gateway/db/settings.go (1)
SettingUpdate(135-138)
web/src/pages/deploy-approval-requests-page.tsx (2)
web/src/hooks/use-mutation.ts (1)
useMutation(65-88)web/src/lib/api.ts (1)
rejectDeployApprovalRequest(184-188)
internal/gateway/settings/groups.go (1)
internal/gateway/settings/defaults.go (1)
GlobalSettingSessionTimeoutMinutes(16-16)
web/src/pages/tokens-page/create-token-dialog.tsx (2)
web/src/pages/tokens-page/use-token-mutations.ts (1)
useTokenMutations(8-63)web/src/contexts/step-up-context.tsx (1)
useStepUp(353-359)
internal/cli/auth.go (3)
internal/cli/types.go (1)
GatewayInfoResponse(103-108)internal/cli/globals.go (1)
HTTPClient(31-31)internal/cli/logging/logging.go (1)
Errorf(20-20)
internal/gateway/auth/session_manager.go (3)
internal/gateway/db/database.go (1)
Database(21-26)internal/gateway/rbac/rbac.go (1)
Database(16-25)internal/gateway/logging/logging.go (2)
Errorf(65-65)Warnf(62-62)
internal/gateway/settings/service_env_helpers.go (2)
internal/gateway/settings/service.go (1)
Service(16-18)internal/gateway/settings/defaults.go (4)
KeyProtectedEnvVars(102-102)KeySecretEnvVars(105-105)KeyApprovedDeployCommands(95-95)KeyServiceImagePatterns(106-106)
internal/gateway/settings/service_session.go (2)
internal/gateway/settings/service.go (1)
Service(16-18)internal/gateway/settings/defaults.go (1)
KeySessionTimeoutMinutes(29-29)
internal/gateway/handlers/settings.go (1)
internal/gateway/settings/groups.go (2)
GlobalSettingGroupKeyStrings(48-55)GlobalSettingGroupSessionConfiguration(14-14)
internal/gateway/handlers/auth_test.go (1)
internal/gateway/auth/session_manager.go (2)
NewSessionManager(67-73)StaticTTLProvider(31-33)
internal/gateway/handlers/auth_mfa_verification_test.go (1)
internal/gateway/auth/session_manager.go (2)
NewSessionManager(67-73)StaticTTLProvider(31-33)
internal/gateway/middleware/auth_test.go (1)
internal/gateway/auth/session_manager.go (2)
NewSessionManager(67-73)StaticTTLProvider(31-33)
web/src/pages/settings-page.tsx (1)
web/src/pages/settings/session-timeout-card.tsx (1)
SessionTimeoutCard(21-172)
web/src/pages/tokens-page/delete-token-dialog.tsx (2)
web/src/pages/tokens-page/use-token-mutations.ts (1)
useTokenMutations(8-63)web/src/contexts/step-up-context.tsx (1)
useStepUp(353-359)
web/src/pages/tokens-page/use-token-mutations.ts (1)
web/src/lib/api.ts (1)
api(428-460)
internal/cli/convox_builds.go (1)
internal/convox/commands.go (1)
Command(13-18)
internal/gateway/handlers/dto.go (2)
internal/gateway/version/version.go (2)
Version(6-6)CommitHash(9-9)internal/cli/globals.go (1)
Version(25-25)
web/src/pages/tokens-page/edit-token-dialog.tsx (2)
web/src/pages/tokens-page/use-token-mutations.ts (1)
useTokenMutations(8-63)web/src/contexts/step-up-context.tsx (1)
useStepUp(353-359)
internal/gateway/handlers/settings_test.go (1)
internal/gateway/auth/session_manager.go (2)
NewSessionManager(67-73)StaticTTLProvider(31-33)
internal/gateway/middleware/mfa_stepup_totp_test.go (1)
internal/gateway/auth/session_manager.go (2)
NewSessionManager(67-73)StaticTTLProvider(31-33)
internal/gateway/app/setup.go (2)
internal/gateway/settings/service.go (1)
NewService(21-23)internal/gateway/auth/session_manager.go (2)
SessionManager(41-45)NewSessionManager(67-73)
🪛 LanguageTool
docs/CONFIGURATION.md
[style] ~29-~29: To form a complete sentence, be sure to include a subject.
Context: ...window for browser sessions in minutes. Can also be configured via the Settings pag...
(MISSING_IT_THERE)
⏰ 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). (4)
- GitHub Check: build-image
- GitHub Check: web-tests
- GitHub Check: lint
- GitHub Check: go-tests
ndbroadbent
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/deploy_approvals_request.go (1)
236-268: Refactor polling loop to avoidtime.Sleepand use a context-aware tickerThe
waitForDeployApprovalfunction usestime.Sleep(interval)at line 266, which is forbidden by the project's golangci-lintforbidigorule. The linting configuration explicitly prohibitstime.Sleepwith the message "avoid time.Sleep; use context with timeout."Refactor to use
time.Tickerdriven bycmd.Context()(optionally wrapped withcontext.WithTimeoutwhentimeout > 0) so that:
- Cancellation via context (e.g., CTRL‑C or parent context cancellation) is honored.
- Timeout is managed by context rather than manual
time.Since(start)checks.- The loop no longer calls
time.Sleepdirectly.Suggested approach:
- Derive a context from
cmd.Context()withcontext.WithTimeoutwhentimeout > 0.- Use a
time.Tickerforinterval.- In the loop,
selectonctx.Done()vs<-ticker.C, performing the status fetch and status/timeout handling on ticker ticks.This change is required to pass the linting checks in CI.
🧹 Nitpick comments (5)
internal/gateway/handlers/deploy_approval_admin.go (1)
48-62: Consider consistent error handling for invalid numeric input.The helper silently ignores non-numeric strings (returns
0, trueon line 55) but explicitly rejects negative values with an error response. This inconsistency could lead to confusing behavior—limit=-5returns a 400 error, butlimit=abcsilently defaults to 0.If silent fallback is intentional (e.g., for backwards compatibility), consider adding a brief comment explaining this design choice. Otherwise, consider returning an error for non-numeric values as well.
internal/cli/deploy_approvals_wait.go (1)
245-247: Consider using context-aware sleep for CLI polling.Per coding guidelines,
time.Sleepis a forbidden pattern—context with timeout is preferred. While this is a CLI command (not a server handler), usingtime.Afterwith context cancellation would allow graceful shutdown on Ctrl+C during the polling loop.+func (w *deployApprovalWaiter) sleep(ctx context.Context) error { + select { + case <-time.After(w.pollInterval): + return nil + case <-ctx.Done(): + return ctx.Err() + } +} -func (w *deployApprovalWaiter) sleep() { - time.Sleep(w.pollInterval) -}Based on coding guidelines regarding
time.Sleepusage.internal/cli/deploy_approvals_list.go (1)
58-68: Consider warning on rack query failures instead of silently continuing.When a gateway request fails for a rack, the error is silently ignored. This could mask connectivity issues or authentication problems. Consider printing a warning so users know which racks failed:
for _, rack := range racks { var result deployApprovalRequestList if err := gatewayRequest(cmd, rack, http.MethodGet, endpoint, nil, &result); err != nil { - // Continue to next rack on error + fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to query rack %s: %v\n", rack, err) continue }internal/cli/deploy_approvals_approve.go (1)
123-127: Clarify error handling for stdin read failures.The error message "aborted" is returned for any read error, including EOF when input is piped. Consider distinguishing:
- if _, err := reader.ReadString('\n'); err != nil { - return fmt.Errorf("aborted") + if _, err := reader.ReadString('\n'); err != nil { + return fmt.Errorf("approval aborted: %w", err) }This provides more context if the command is used non-interactively.
internal/cli/deploy_approvals_show.go (1)
128-152: Consider consolidating withfindPendingRequestto reduce duplication.This function shares significant logic with
findPendingRequestindeploy_approvals_approve.go—both build query parameters, iterate racks, and return the first matching request. The main difference is the return signature.Consider extracting a shared helper that both can use, or unifying the signatures. This would reduce duplication (~20 lines) and ensure consistent behavior.
Example unified helper:
func findRequest(cmd *cobra.Command, racks []string, status, branch, commit string) (*deployApprovalRequest, string, error) { // ... shared logic ... // Returns (request, rack, nil) on success, (nil, "", nil) if not found }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.deepsource.toml(1 hunks)CLAUDE.md(1 hunks)internal/cli/deploy_approvals_approve.go(2 hunks)internal/cli/deploy_approvals_command.go(1 hunks)internal/cli/deploy_approvals_list.go(1 hunks)internal/cli/deploy_approvals_request.go(1 hunks)internal/cli/deploy_approvals_shared.go(2 hunks)internal/cli/deploy_approvals_show.go(1 hunks)internal/cli/deploy_approvals_wait.go(1 hunks)internal/gateway/db/deploy_approval_requests_queries.go(2 hunks)internal/gateway/db/settings.go(3 hunks)internal/gateway/handlers/deploy_approval_admin.go(1 hunks)web/.eslintrc.json(1 hunks)web/vite.config.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- web/.eslintrc.json
- .deepsource.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/deploy_approvals_command.go
- web/vite.config.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Maximum 500 lines per Go code file - refactor immediately if approaching this limit or split into smaller, focused modules
Maximum 100 lines per function (Go) - enforced by golangci-lint using funlen linter
Maximum 50 statements per function (Go) - enforced by golangci-lint
Maximum cognitive complexity of 15 (Go) - enforced by golangci-lint using gocognit linter
Line length maximum 120 characters - enforced by golangci-lint lll rule. Exception: For Go struct tag lines that cannot be broken without invalidating formatting (where gofmt/goimports always rejoin them and field+tag exceeds 120 chars), add//nolint:lllat end of line only
No//nolintcomments allowed except for struct tag line length exceptions - fix the code instead of suppressing warnings
Forbidden patterns in Go:panic(use error returns),fmt.Print*except in CLI output (use proper logging),time.Sleep(use context with timeout)
Zero code duplication allowed in production code - threshold 10 lines or 50 tokens, enforced by jscpd
Usegovulncheckto check for known Go vulnerabilities - required security scan
Files:
internal/gateway/db/settings.gointernal/cli/deploy_approvals_list.gointernal/gateway/handlers/deploy_approval_admin.gointernal/gateway/db/deploy_approval_requests_queries.gointernal/cli/deploy_approvals_shared.gointernal/cli/deploy_approvals_request.gointernal/cli/deploy_approvals_approve.gointernal/cli/deploy_approvals_wait.gointernal/cli/deploy_approvals_show.go
internal/gateway/db/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Database layer code should follow Repository pattern for data access abstractions
Files:
internal/gateway/db/settings.gointernal/gateway/db/deploy_approval_requests_queries.go
internal/gateway/**/*.go
📄 CodeRabbit inference engine (internal/gateway/CLAUDE.md)
Use audit logger for all sensitive data in Go files
Files:
internal/gateway/db/settings.gointernal/gateway/handlers/deploy_approval_admin.gointernal/gateway/db/deploy_approval_requests_queries.go
🧬 Code graph analysis (5)
internal/gateway/db/settings.go (1)
internal/gateway/db/database.go (1)
Database(21-26)
internal/gateway/handlers/deploy_approval_admin.go (2)
internal/gateway/db/deploy_approval_requests_queries.go (1)
DeployApprovalRequestListOptions(82-90)internal/gateway/openapi/serve.go (1)
JSON(26-28)
internal/cli/deploy_approvals_shared.go (1)
internal/cli/rack.go (1)
SelectedRack(11-32)
internal/cli/deploy_approvals_request.go (1)
internal/cli/rack.go (1)
SelectedRack(11-32)
internal/cli/deploy_approvals_approve.go (2)
internal/convox/commands.go (1)
Command(13-18)internal/cli/utils.go (1)
SilenceOnError(17-26)
⏰ 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). (4)
- GitHub Check: web-tests
- GitHub Check: go-tests
- GitHub Check: lint
- GitHub Check: build-image
🔇 Additional comments (18)
CLAUDE.md (1)
92-101: Clear and actionable DeepSource documentation.The new section effectively explains the distinction between local Biome linting and CI-based DeepSource ESLint analysis, with concrete guidance on where and how to disable rules. The example of
react/jsx-no-bindwith rationale is helpful for developers.internal/gateway/handlers/deploy_approval_admin.go (1)
25-46: LGTM!The refactored option parsing is cleaner with composite literal initialization and proper use of the
parseNonNegativeInthelper for validation.internal/gateway/db/deploy_approval_requests_queries.go (1)
82-90: LGTM!The new
GitBranchandGitCommitHashfilter fields are cleanly integrated following the existing pattern of conditional WHERE clause construction with parameterized queries.Also applies to: 109-116
internal/cli/deploy_approvals_shared.go (3)
99-108: Verify//nolint:gosecusage aligns with project policy.Per coding guidelines,
//nolintcomments are generally not allowed except for struct tag line length exceptions. This//nolint:gosecsuppresses G204 (subprocess launched with variable) which is a false positive here since the command is hardcoded.Consider whether this exception is acceptable under project policy, or if there's an alternative approach (e.g., adding the file to gosec's exclude list).
Based on coding guidelines regarding nolint usage.
74-97: LGTM!The
resolveRackshelper properly handles empty input, comma-separated values, and whitespace trimming. Good defensive programming with the empty racks check at the end.
110-124: LGTM!The fallback logic to current git branch when neither branch nor commit is specified is clean and provides good UX for the common case.
internal/cli/deploy_approvals_wait.go (1)
62-65: LGTM!Good refactoring to use the centralized
resolveRackshelper, reducing code duplication.internal/cli/deploy_approvals_list.go (2)
82-101: LGTM!Clean URL construction using
url.Valuesfor proper parameter encoding.
103-152: LGTM!The table formatting handles both single and multi-rack display modes well, with appropriate truncation for long values.
internal/cli/deploy_approvals_approve.go (2)
63-87: LGTM!The dual-mode execution flow (ID-based vs search-based) is cleanly implemented with proper UUID validation for direct approval.
108-141: LGTM!The search-based approval flow with interactive confirmation provides good UX. The error messages correctly report which criterion (branch or commit) was used for the search.
internal/cli/deploy_approvals_show.go (2)
61-85: LGTM!The execution flow cleanly handles both ID-based and search-based lookups with proper UUID validation.
154-191: LGTM!Comprehensive output formatting with proper handling of optional fields (token name, approval timestamps, notes).
internal/gateway/db/settings.go (4)
68-90: LGTM! SQL constants successfully eliminate duplication.The extraction of SQL upsert statements into named constants addresses the code duplication concern from the previous review. The SQL syntax is correct for PostgreSQL, including the
COALESCE(app_name, '')for handling NULL in the unique constraint and the?::jsonbcast for proper type handling.
119-134: LGTM! Clean delegation to centralized core function.The
UpsertSettingmethod now correctly delegates toupsertSettingCorewithniltransaction parameter, maintaining its non-transactional behavior while eliminating code duplication.
136-174: LGTM! Well-structured atomic batch update operation.The new
UpsertSettingsInTxfunction provides atomic batch update capability with correct transaction handling:
- Early return for empty updates (defensive programming)
- Proper transaction lifecycle:
Begin→ deferredRollback→ explicitCommit- Consistent error wrapping with context (scope and key)
- Leverages the centralized
upsertSettingCorefor each settingThe transaction pattern ensures all-or-nothing semantics for batch updates, which is essential for maintaining configuration consistency.
92-117: execTx method exists with correct signature—refactoring is sound.The
execTxmethod is properly defined ininternal/gateway/db/database.go(line 304) with the expected signaturefunc (d *Database) execTx(tx *sql.Tx, q string, args ...interface{}) (sql.Result, error). The implementation correctly delegates totx.Exec()after logging and rebinding the query. The centralizedupsertSettingCorefunction successfully eliminates duplication by abstracting transaction vs. non-transaction execution paths, and all method calls are valid.internal/cli/deploy_approvals_request.go (1)
79-101: Centralizing rack resolution viaSelectedRackis consistent with other CLI commandsUsing
SelectedRack()here instead of a per-command rack flag brings this command in line with the shared rack selection logic used elsewhere (env, login state, global--rack), and the error path clearly surfaces when no rack is selected. This looks correct and simplifies configuration flow for deploy approval requests.
1 participant
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.