cosmetic changes to clean up app history table. #532
Conversation
📝 WalkthroughWalkthroughAlways use the current cluster context for app history; replace per-cluster switching with a single cluster view. Change Limit from int32 to int (default 10) and add a public All bool flag to disable limiting. Introduce deployment sorting (active first, then recent) and detect cluster identity presence. Update table rendering to be identity-aware and conditional on Detailed mode, and adjust no-deployments messaging and minor wiring/formatting. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/commands/app_history.go (1)
32-40: Guardopts.Limitbefore castingint → int32to prevent silent overflow.Casting a user-provided
inttoint32without validation risks silent wrapping. A value exceeding 2,147,483,647 (MaxInt32) will wrap to a negative or incorrect int32, potentially producing unexpected behavior despite validation being server-side.Proposed fix (fail fast or clamp)
import ( "fmt" + "math" "sort" "strings" "time" @@ func AppHistory(ctx *Context, opts struct { @@ }) error { @@ + if opts.Limit < 0 { + return fmt.Errorf("--limit must be >= 0") + } + if opts.Limit > math.MaxInt32 { + opts.Limit = math.MaxInt32 + } + result, err := depClient.ListDeployments(ctx, opts.App, clusterId, opts.Status, int32(opts.Limit)) if err != nil { return fmt.Errorf("failed to list deployments: %w", err) }Run
make lint-fixafter adjusting imports.Also applies to: line 52
🧹 Nitpick comments (2)
cli/commands/app_history.go (2)
74-76: Sorting behavior matches the PR intent; consider stable sort as a minor polish.Active-first + most-recent-by-time is implemented clearly. If you want deterministic output when timestamps tie (or are missing),
sort.SliceStablewould preserve original order for ties.Also applies to: 122-140
147-168: Dynamic headers/column indices look correct; consider derivingidColIndexfrom headers for resilience.The
idColIndexadjustment forDEPLOYED BYis correct as written, but it’s a bit brittle if columns evolve again. Computing the index by searchingheaders(or using constants) would reduce future off-by-one risk.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cli/commands/app_history.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Always runmake lintbefore committing - runs golangci-lint on the entire codebase
Runmake lint-fixto automatically fix linting issues where possible
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Function/method comments should explain the purpose and any important side effects, not just restate the name
Codebase follows standard Go formatting conventions
Files:
cli/commands/app_history.go
🧠 Learnings (1)
📚 Learning: 2025-12-15T17:54:35.854Z
Learnt from: phinze
Repo: mirendev/runtime PR: 478
File: cli/commands/debug_netdb.go:155-156
Timestamp: 2025-12-15T17:54:35.854Z
Learning: In reviews of Go CLI commands under cli/commands (mirendev/runtime), prefer validating inputs on the server side and skip duplicating client-side validation when the CLI is the only client. This avoids code duplication and keeps validation centralized. If local validation is absolutely necessary for UX, keep it slim and clearly justified, and ensure it does not duplicate server logic. Document the rationale and ensure server API validation remains the source of truth.
Applied to files:
cli/commands/app_history.go
🧬 Code graph analysis (1)
cli/commands/app_history.go (4)
clientconfig/clientconfig.go (1)
ClusterConfig(43-52)api/deployment/deployment_v1alpha/rpc.gen.go (1)
DeploymentInfo(30-32)pkg/ui/table.go (3)
Row(76-76)ColumnBuilder(32-34)Columns(37-39)pkg/ui/entity.go (1)
CleanEntityID(11-29)
⏰ 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). (5)
- GitHub Check: test
- GitHub Check: build-binaries (darwin, amd64, macos-latest)
- GitHub Check: test-e2e
- GitHub Check: build-binaries (darwin, arm64, macos-latest)
- GitHub Check: lint
🔇 Additional comments (2)
cli/commands/app_history.go (2)
3-12: Import change looks fine.
sortis used bysortDeployments, so this is clean.
178-199: Row construction aligns with conditional columns (identity + detailed).The ordering matches the headers in both modes, and
formatUser()safely falls back to-.
teresamychu
force-pushed
the
app-history-cosmetic
branch
from
3caa380 to
16c47b1
Compare
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)
cli/commands/app_history.go (1)
32-49: Validate--limitbefore convertingint→int32(overflow/negative hazards).Right now
int32(opts.Limit)can wrap on large values (and allows negative), which can yield surprising server behavior. Consider validating and converting once.Proposed fix
import ( "fmt" + "math" "sort" "strings" "time" @@ func AppHistory(ctx *Context, opts struct { @@ }) error { + if opts.Limit < 0 { + return fmt.Errorf("--limit must be >= 0") + } + if opts.Limit > math.MaxInt32 { + return fmt.Errorf("--limit must be <= %d", math.MaxInt32) + } + limit := int32(opts.Limit) + depCl, err := ctx.RPCClient("dev.miren.runtime/deployment") if err != nil { return fmt.Errorf("failed to connect to deployment service: %w", err) } depClient := deployment_v1alpha.NewDeploymentClient(depCl) - result, err := depClient.ListDeployments(ctx, opts.App, ctx.ClusterName, opts.Status, int32(opts.Limit)) + result, err := depClient.ListDeployments(ctx, opts.App, ctx.ClusterName, opts.Status, limit) if err != nil { return fmt.Errorf("failed to list deployments: %w", err) }
🧹 Nitpick comments (1)
cli/commands/app_history.go (1)
68-129: Sorting looks right; considersort.SliceStable+ deterministic tie-breaker.The “active first, then most recent” logic matches the PR intent. For deterministic output when timestamps/statuses match, I’d lean towards
sort.SliceStableand a final tie-breaker (e.g.,Id()).Optional refinement
func sortDeployments(deployments []*deployment_v1alpha.DeploymentInfo) { - sort.Slice(deployments, func(i, j int) bool { + sort.SliceStable(deployments, func(i, j int) bool { @@ - return iTime > jTime + if iTime != jTime { + return iTime > jTime + } + return deployments[i].Id() > deployments[j].Id() }) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cli/commands/app_history.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Always runmake lintbefore committing - runs golangci-lint on the entire codebase
Runmake lint-fixto automatically fix linting issues where possible
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Function/method comments should explain the purpose and any important side effects, not just restate the name
Codebase follows standard Go formatting conventions
Files:
cli/commands/app_history.go
🧠 Learnings (1)
📚 Learning: 2025-12-15T17:54:35.854Z
Learnt from: phinze
Repo: mirendev/runtime PR: 478
File: cli/commands/debug_netdb.go:155-156
Timestamp: 2025-12-15T17:54:35.854Z
Learning: In reviews of Go CLI commands under cli/commands (mirendev/runtime), prefer validating inputs on the server side and skip duplicating client-side validation when the CLI is the only client. This avoids code duplication and keeps validation centralized. If local validation is absolutely necessary for UX, keep it slim and clearly justified, and ensure it does not duplicate server logic. Document the rationale and ensure server API validation remains the source of truth.
Applied to files:
cli/commands/app_history.go
⏰ 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: lint
- GitHub Check: build-binaries (darwin, arm64, macos-latest)
- GitHub Check: test
- GitHub Check: test-e2e
🔇 Additional comments (5)
cli/commands/app_history.go (5)
131-157: Dynamic headers/column indexes are consistent (nice cleanup).Header construction matches the row shape, and the
idColIndexadjustment whenDEPLOYED BYis present is correct.
167-186: Row composition order matches headers; conditional identity insertion is clean.The row ordering (
STATUS,VERSION, optionalDEPLOYED BY,WHEN, then detailed vs compact tail columns) aligns withbuildDeploymentTable().
90-99: No-deployments message is clearer with cluster + status context.This reads well and matches the new cluster-scoped behavior.
46-76: Confirmctx.ClusterNameis always set now that “All clusters” is removed.
ListDeployments(..., ctx.ClusterName, ...)+ the “Cluster: …” header will look broken (or change semantics) ifctx.ClusterName == "". If that’s possible, consider failing fast with a clear error (or explicitly mapping empty to a known default).#!/bin/bash set -euo pipefail # 1) Find Context definition and how ClusterName is set. rg -n --hidden -S 'type Context struct|ClusterName|ClusterConfig' cli # 2) Find AppHistory call sites / command wiring to see if ClusterName can be empty here. rg -n --hidden -S '\bAppHistory\s*\(' cli
77-80: ThehasIdentitycondition is correct and aligns with proper "identity configured" semantics. The expressionctx.ClusterConfig != nil && (ctx.ClusterConfig.Identity != "" || ctx.ClusterConfig.CloudAuth)appropriately checks whether identity authentication is configured, which is the right basis for deciding whether to display the "DEPLOYED BY" column.The deployed-by data comes directly from the deployment entity (not from ClientConfig), and
formatUser()safely returns"-"for any missing or placeholder values, eliminating the risk of showing empty columns.
teresamychu
force-pushed
the
app-history-cosmetic
branch
2 times, most recently
from
8b6b93f to
2b873f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/commands/app_history.go (1)
156-166: Consider simplifying the ID column index computation.The loop to find
idColIndexworks correctly, but since you control the header construction, you could compute the index directly:idColIndex := len(headers) // capture before appending ID headers = append(headers, "WHEN", "ID", "ERROR")That said, the current search approach is more resilient to future header reordering.
♻️ Optional simplification
} else { headers = []string{"STATUS", "VERSION"} if opts.hasIdentity { headers = append(headers, "DEPLOYED BY") } - headers = append(headers, "WHEN", "ID", "ERROR") - // Find ID column index dynamically - idColIndex := -1 - for i, h := range headers { - if h == "ID" { - idColIndex = i - break - } - } + headers = append(headers, "WHEN") + idColIndex := len(headers) + headers = append(headers, "ID", "ERROR") builder = ui.Columns(). NoTruncate(0, idColIndex) // STATUS and ID }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cli/commands/app_history.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Always runmake lintbefore committing - runs golangci-lint on the entire codebase
Runmake lint-fixto automatically fix linting issues where possible
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Function/method comments should explain the purpose and any important side effects, not just restate the name
Codebase follows standard Go formatting conventions
Files:
cli/commands/app_history.go
🧠 Learnings (1)
📚 Learning: 2025-12-15T17:54:35.854Z
Learnt from: phinze
Repo: mirendev/runtime PR: 478
File: cli/commands/debug_netdb.go:155-156
Timestamp: 2025-12-15T17:54:35.854Z
Learning: In reviews of Go CLI commands under cli/commands (mirendev/runtime), prefer validating inputs on the server side and skip duplicating client-side validation when the CLI is the only client. This avoids code duplication and keeps validation centralized. If local validation is absolutely necessary for UX, keep it slim and clearly justified, and ensure it does not duplicate server logic. Document the rationale and ensure server API validation remains the source of truth.
Applied to files:
cli/commands/app_history.go
🔇 Additional comments (6)
cli/commands/app_history.go (6)
27-30: LGTM!The
hasIdentityfield cleanly encapsulates the identity-awareness for conditional column rendering throughout the display logic.
35-36: Good fix for the--limitflag.Changing from
int32tointresolves the flag parsing issue, and the new--allflag provides a clean way to bypass the limit.
117-135: LGTM!The sorting logic correctly prioritizes active deployments first, then orders by most recent. The defensive nil checks on
DeployedAtprevent potential panics.
177-199: LGTM!The row construction correctly mirrors the conditional header structure, ensuring column alignment regardless of identity configuration.
96-105: LGTM!Clear and informative messaging that now includes cluster context for better user feedback.
83-86: LGTM!The identity detection logic properly guards against nil
ClusterConfigand correctly identifies clusters with identity configured via either direct identity or cloud auth.
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
One flag collision to address and a couple of nonblocking Qs for ya!
teresamychu
force-pushed
the
app-history-cosmetic
branch
from
2b873f8 to
447684f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cli/commands/app_history.go:
- Around line 117-129: Update sortDeployments to prioritize active deployments
first: in the sort.Slice comparator (function sortDeployments) first check each
deployment's active flag (use the proto accessors available on DeploymentInfo
such as HasActive()/GetActive() or the Active field) and if one is active and
the other not, return the active one as "less" (i.e., put it before); if both
have the same active status, fall back to the existing time comparison using
DeployedAt().Seconds() with the same nil checks. Ensure you handle nil/absent
Active and DeployedAt fields consistently to avoid panics.
🧹 Nitpick comments (1)
cli/commands/app_history.go (1)
152-161: Consider simplifying ID column index logic.The dynamic search for the ID column index is correct but verbose. Since the headers are constructed in a predictable order, you could calculate the index directly.
♻️ Simplified approach
- // Find ID column index dynamically - idColIndex := -1 - for i, h := range headers { - if h == "ID" { - idColIndex = i - break - } - } + // ID column index: STATUS, VERSION, [DEPLOYED BY], WHEN, ID + idColIndex := 4 + if opts.hasIdentity { + idColIndex = 5 + } builder = ui.Columns(). NoTruncate(0, idColIndex) // STATUS and ID
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cli/commands/app_history.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Always runmake lintbefore committing - runs golangci-lint on the entire codebase
Runmake lint-fixto automatically fix linting issues where possible
Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does
Function/method comments should explain the purpose and any important side effects, not just restate the name
Codebase follows standard Go formatting conventions
Files:
cli/commands/app_history.go
🧠 Learnings (2)
📚 Learning: 2025-08-18T21:50:46.874Z
Learnt from: phinze
Repo: mirendev/runtime PR: 181
File: cli/commands/logs.go:104-109
Timestamp: 2025-08-18T21:50:46.874Z
Learning: In the observability/logs.go server implementation, both Read and ReadBySandbox methods use `>=` comparison for timestamp filtering, not `>`. This means client-side pagination should add a small time increment (like 1 microsecond) to avoid duplicate logs in subsequent batches.
Applied to files:
cli/commands/app_history.go
📚 Learning: 2025-12-15T17:54:35.854Z
Learnt from: phinze
Repo: mirendev/runtime PR: 478
File: cli/commands/debug_netdb.go:155-156
Timestamp: 2025-12-15T17:54:35.854Z
Learning: In reviews of Go CLI commands under cli/commands (mirendev/runtime), prefer validating inputs on the server side and skip duplicating client-side validation when the CLI is the only client. This avoids code duplication and keeps validation centralized. If local validation is absolutely necessary for UX, keep it slim and clearly justified, and ensure it does not duplicate server logic. Document the rationale and ensure server API validation remains the source of truth.
Applied to files:
cli/commands/app_history.go
⏰ 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-binaries (darwin, arm64, macos-latest)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: test-e2e
Teresa's Notes
This PR cleans up the app history table to make it more readable with some minor changes.
Also found a bug that the limit (-n) flag was actually never working because it was declared as
int32Heres what it looks in this PR:
This concludes the fixes for MIR-607
Summary
Cosmetic Changes
limitto 10activedeployment at the top, then by time (most recent first)DEPLOYED BYcolumn when cluster has no identity configuredBug Fixes
--limitflag--allflag to show all deployments for this cluster only.Test plan