Add regions names - #171
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughImplements a two-step region selection flow: broad-area selection followed by region-specific choice, fetching region display names from AWS SSM in batches; adds region grouping helper and improves error handling. Also replaces string concatenation with a strings.Builder in the UI table rendering. Changes
Sequence DiagramsequenceDiagram
actor User
participant SelectRegion as SelectRegion()
participant getDisplay as getRegionDisplayNames()
participant AWS as AWS SSM
participant Form as Two-Step Form
participant Broad as getBroadRegion()
User->>SelectRegion: start selection
SelectRegion->>getDisplay: fetch display names (batched)
getDisplay->>AWS: request longName for region codes
AWS-->>getDisplay: return longName map
getDisplay-->>SelectRegion: displayNames map
SelectRegion->>Form: render step 1 (broad regions)
Form->>User: show broad-area options
User->>Form: choose broad area
Form-->>SelectRegion: selected broad
SelectRegion->>Broad: determine regions in broad area
Broad-->>SelectRegion: region codes list
SelectRegion->>Form: render step 2 (region options with displayNames)
Form->>User: show specific region options
User->>Form: choose region
Form-->>SelectRegion: selected region code
SelectRegion-->>User: confirm selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR enhances region selection by grouping AWS regions into broad geographic areas and showing human-readable region names (via SSM), and also refactors UI status table generation to use a builder.
Changes:
- Add broad-region categorization and a two-step interactive region selection flow.
- Fetch AWS region long names from SSM to display friendlier labels in the selector.
- Replace per-row string concatenation in the UI status endpoint with a
strings.Builder.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tailout/ui.go |
Refactors /status HTML table generation to use a builder instead of repeated string concatenation. |
internal/common.go |
Adds region display-name lookup (SSM) and updates SelectRegion to a broad-area + specific-region selection flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| table := "" | ||
| var tableSb65 strings.Builder | ||
| for _, node := range nodes { | ||
| table += fmt.Sprintf("<tr class=\"bg-white border-b\"><td class=\"px-4 py-2\">%s</td><td class=\"px-4 py-2\">%s</td><td class=\"px-4 py-2\">%s</td></tr>", node.Hostname, node.Addresses[0], node.LastSeen) | ||
| tableSb65.WriteString(fmt.Sprintf("<tr class=\"bg-white border-b\"><td class=\"px-4 py-2\">%s</td><td class=\"px-4 py-2\">%s</td><td class=\"px-4 py-2\">%s</td></tr>", node.Hostname, node.Addresses[0], node.LastSeen)) | ||
| } | ||
| table += tableSb65.String() |
There was a problem hiding this comment.
The new builder is named tableSb65 and the code still builds into a separate table string via table += .... This makes the change harder to read and keeps unnecessary string state. Consider using a clearly named strings.Builder (e.g., var tableSB strings.Builder) and write the builder output directly to the response (or assign table = tableSB.String()), removing the table += pattern here.
| for _, param := range output.Parameters { | ||
| // Path: /aws/service/global-infrastructure/regions/{code}/longName | ||
| parts := strings.Split(*param.Name, "/") | ||
| if len(parts) >= 7 { | ||
| names[parts[5]] = *param.Value | ||
| } |
There was a problem hiding this comment.
getRegionDisplayNames dereferences param.Name and param.Value without nil checks. The AWS SDK's ssm.Parameter fields are pointers and can be nil, which would panic here. Please guard against nil (and skip/handle parameters with missing Name/Value) before splitting/parsing.
| // Step 1: pick broad area. | ||
| huh.NewGroup( | ||
| huh.NewSelect[string](). | ||
| Title("Select a region"). |
There was a problem hiding this comment.
In the first step you're selecting a broad geographic area, but the prompt title still says "Select a region". This is misleading given the updated two-step flow; consider changing the title to reflect that it's selecting the broad area (e.g., "Select a geographic area").
| Title("Select a region"). | |
| Title("Select a geographic area"). |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/common.go`:
- Around line 142-148: The call to getRegionDisplayNames makes region selection
fail when SSM lookup errors; change the error handling in internal/common.go so
that if getRegionDisplayNames returns an error you only return if ctx.Err() !=
nil (preserve cancellation), otherwise log or swallow the error and continue (do
not return the error) so displayNames can be nil/empty and the downstream
OptionsFunc can fall back to raw region codes; update the block around
getRegionDisplayNames(ctx, regionCodes) accordingly and ensure logging includes
the error for observability.
- Around line 79-84: The loop over output.Parameters dereferences ssm.Parameter
pointer fields (param.Name and param.Value) without nil checks which can panic;
update the loop in internal/common.go to guard both fields (e.g., if param ==
nil || param.Name == nil || param.Value == nil { continue }) before using
*param.Name and *param.Value, then proceed to split *param.Name and assign
names[parts[5]] = *param.Value only when the nil checks pass and the parts
length check still holds.
In `@tailout/ui.go`:
- Around line 66-71: The table row builder unconditionally indexes
node.Addresses[0] and injects raw values into HTML; update the loop that builds
tableSb65 to first guard against empty address slices (use a safeAddress
variable set to a placeholder like "-" when len(node.Addresses) == 0) and escape
any user-controlled strings (Hostname, safeAddress, LastSeen) before
concatenation (use html.EscapeString or the html/template package) so writes via
w.Write produce safe, non-panicky HTML rows.
| displayNames, err := getRegionDisplayNames(ctx, regionCodes) | ||
| if err != nil { | ||
| if ctx.Err() != nil { | ||
| return "", fmt.Errorf("region selection canceled: %w", ctx.Err()) | ||
| } | ||
| return "", fmt.Errorf("failed to get region names: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the file and examine the broader context
find . -name "common.go" -type fRepository: lucacome/tailout
Length of output: 80
🏁 Script executed:
# Search for getRegionDisplayNames function definition
rg "func.*getRegionDisplayNames" --context 5Repository: lucacome/tailout
Length of output: 712
🏁 Script executed:
# Look at how displayNames is used after the error handling
rg -A 20 "getRegionDisplayNames" internal/common.goRepository: lucacome/tailout
Length of output: 1586
🏁 Script executed:
# Get more context on how displayNames is used after the error handling
rg -A 50 "displayNames, err := getRegionDisplayNames" internal/common.go | head -80Repository: lucacome/tailout
Length of output: 1691
🏁 Script executed:
# Search for where displayNames is referenced in the region selection logic
rg "displayNames" internal/common.go -B 2 -A 2Repository: lucacome/tailout
Length of output: 401
Don't make region selection fail when SSM display-name lookup fails.
Line 148 currently turns display-name enrichment into a hard dependency. If SSM access is denied/unavailable, users can't select any region even though region codes are available. The downstream OptionsFunc already handles missing display names by falling back to raw region codes, so the SSM failure should not be fatal.
Suggested fix
displayNames, err := getRegionDisplayNames(ctx, regionCodes)
if err != nil {
if ctx.Err() != nil {
return "", fmt.Errorf("region selection canceled: %w", ctx.Err())
}
- return "", fmt.Errorf("failed to get region names: %w", err)
+ // Keep selection functional with raw codes when display-name enrichment fails.
+ displayNames = map[string]string{}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common.go` around lines 142 - 148, The call to getRegionDisplayNames
makes region selection fail when SSM lookup errors; change the error handling in
internal/common.go so that if getRegionDisplayNames returns an error you only
return if ctx.Err() != nil (preserve cancellation), otherwise log or swallow the
error and continue (do not return the error) so displayNames can be nil/empty
and the downstream OptionsFunc can fall back to raw region codes; update the
block around getRegionDisplayNames(ctx, regionCodes) accordingly and ensure
logging includes the error for observability.
| var tableSb65 strings.Builder | ||
| for _, node := range nodes { | ||
| table += fmt.Sprintf("<tr class=\"bg-white border-b\"><td class=\"px-4 py-2\">%s</td><td class=\"px-4 py-2\">%s</td><td class=\"px-4 py-2\">%s</td></tr>", node.Hostname, node.Addresses[0], node.LastSeen) | ||
| tableSb65.WriteString(fmt.Sprintf("<tr class=\"bg-white border-b\"><td class=\"px-4 py-2\">%s</td><td class=\"px-4 py-2\">%s</td><td class=\"px-4 py-2\">%s</td></tr>", node.Hostname, node.Addresses[0], node.LastSeen)) | ||
| } | ||
| table += tableSb65.String() | ||
| if _, err := w.Write([]byte(table)); err != nil { |
There was a problem hiding this comment.
Guard slice access and escape HTML in the status table rows.
Line 68 directly indexes node.Addresses[0] and writes unescaped values into HTML. This can panic on empty addresses and allows HTML/script injection from node metadata.
Suggested fix
import (
"context"
"fmt"
+ "html"
"log/slog"
"net/http"
"net/url"
"strings"
"time"
@@
- table := ""
- var tableSb65 strings.Builder
+ var tableSb strings.Builder
for _, node := range nodes {
- tableSb65.WriteString(fmt.Sprintf("<tr class=\"bg-white border-b\"><td class=\"px-4 py-2\">%s</td><td class=\"px-4 py-2\">%s</td><td class=\"px-4 py-2\">%s</td></tr>", node.Hostname, node.Addresses[0], node.LastSeen))
+ addr := ""
+ if len(node.Addresses) > 0 {
+ addr = node.Addresses[0]
+ }
+ tableSb.WriteString(fmt.Sprintf(
+ `<tr class="bg-white border-b"><td class="px-4 py-2">%s</td><td class="px-4 py-2">%s</td><td class="px-4 py-2">%s</td></tr>`,
+ html.EscapeString(node.Hostname),
+ html.EscapeString(addr),
+ html.EscapeString(fmt.Sprint(node.LastSeen)),
+ ))
}
- table += tableSb65.String()
- if _, err := w.Write([]byte(table)); err != nil {
+ if _, err := w.Write([]byte(tableSb.String())); err != nil {
slog.Error("failed to write response", "error", err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tailout/ui.go` around lines 66 - 71, The table row builder unconditionally
indexes node.Addresses[0] and injects raw values into HTML; update the loop that
builds tableSb65 to first guard against empty address slices (use a safeAddress
variable set to a placeholder like "-" when len(node.Addresses) == 0) and escape
any user-controlled strings (Hostname, safeAddress, LastSeen) before
concatenation (use html.EscapeString or the html/template package) so writes via
w.Write produce safe, non-panicky HTML rows.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/common.go (1)
145-151:⚠️ Potential issue | 🟠 MajorMake display-name enrichment non-fatal to keep region selection usable.
Line 150 still returns an error when SSM name lookup fails. That makes region selection fail even though region codes are already available and the options builder already supports raw-code fallback.
Proposed fix
displayNames, err := getRegionDisplayNames(ctx, regionCodes) if err != nil { if ctx.Err() != nil { return "", fmt.Errorf("region selection canceled: %w", ctx.Err()) } - return "", fmt.Errorf("failed to get region names: %w", err) + // Non-fatal: continue with raw region codes. + displayNames = map[string]string{} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common.go` around lines 145 - 151, The call to getRegionDisplayNames should not make region selection fail when it returns an error: modify the error branch so that if ctx.Err() != nil you still return the cancellation error, but otherwise do NOT return the SSM lookup error; instead log or ignore the error, leave displayNames as nil/empty and continue so the existing options builder can fall back to using regionCodes. Update the error handling around getRegionDisplayNames (references: getRegionDisplayNames, displayNames, regionCodes, ctx.Err()) to treat lookup failures as non-fatal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/views/components/title_templ.go`:
- Line 3: CI is failing because the workflow pins templ to v0.3.960 while
generated files and go.mod use v0.3.977; update the templ version string in the
CI workflow file (ci.yaml) from "v0.3.960" to "v0.3.977" so the "Check if templ
code is up to date" step uses the same version as the generated headers and
go.mod, then rerun the templ generation step to confirm no diffs.
---
Duplicate comments:
In `@internal/common.go`:
- Around line 145-151: The call to getRegionDisplayNames should not make region
selection fail when it returns an error: modify the error branch so that if
ctx.Err() != nil you still return the cancellation error, but otherwise do NOT
return the SSM lookup error; instead log or ignore the error, leave displayNames
as nil/empty and continue so the existing options builder can fall back to using
regionCodes. Update the error handling around getRegionDisplayNames (references:
getRegionDisplayNames, displayNames, regionCodes, ctx.Err()) to treat lookup
failures as non-fatal.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/common.gointernal/views/components/footer_templ.gointernal/views/components/header_templ.gointernal/views/components/title_templ.gointernal/views/index_templ.gotailout/ui.go
✅ Files skipped from review due to trivial changes (3)
- internal/views/index_templ.go
- internal/views/components/footer_templ.go
- internal/views/components/header_templ.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tailout/ui.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ctx.Err() != nil { | ||
| return "", fmt.Errorf("region selection canceled: %w", ctx.Err()) | ||
| } | ||
| return "", fmt.Errorf("failed to get region names: %w", err) |
There was a problem hiding this comment.
getRegionDisplayNames failures currently abort region selection. This introduces a hard dependency on ssm:GetParameters permissions (and SSM availability), which can break users who previously only needed ec2:DescribeRegions. Consider treating SSM lookup as best-effort (return an empty/partial name map on AccessDenied/network errors and fall back to showing region codes) so the core flow still works.
| if ctx.Err() != nil { | |
| return "", fmt.Errorf("region selection canceled: %w", ctx.Err()) | |
| } | |
| return "", fmt.Errorf("failed to get region names: %w", err) | |
| // If the context was canceled, respect that and abort. | |
| if ctx.Err() != nil { | |
| return "", fmt.Errorf("region selection canceled: %w", ctx.Err()) | |
| } | |
| // Otherwise, treat display-name lookup as best-effort: fall back to codes only. | |
| displayNames = map[string]string{} |
| cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion("us-east-1")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load config: %w", err) | ||
| } | ||
| ssmSvc := ssm.NewFromConfig(cfg) |
There was a problem hiding this comment.
SelectRegion loads AWS config twice (once in GetRegions, again in getRegionDisplayNames). If this runs on every interactive create, it adds extra latency and repeated config/credential resolution. Consider reusing the loaded cfg (or passing an ssm.Client/aws.Config into helpers) to avoid duplicate loads.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Summary by CodeRabbit
New Features
Performance