Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
📝 WalkthroughWalkthroughA new feedback submission feature is introduced, allowing users to run Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as lstk feedback<br/>(cmd/feedback.go)
participant TUI as Bubble Tea UI<br/>(internal/ui)
participant Client as Linear Client<br/>(internal/feedback)
participant OAuth as OAuth Provider<br/>(Linear)
participant GraphQL as Linear GraphQL<br/>API
User->>CLI: Run lstk feedback
CLI->>CLI: Validate interactive terminal<br/>and Linear credentials
CLI->>TUI: Start interactive feedback form
User->>TUI: Enter feedback message
TUI->>TUI: Display message preview<br/>and metadata
User->>TUI: Confirm submission [Y/n]
TUI->>Client: Submit feedback with context
Client->>OAuth: POST /oauth/token<br/>(client credentials)
OAuth-->>Client: Access token
Client->>GraphQL: Query: Find "CLI" label ID
GraphQL-->>Client: Label ID (if exists)
Client->>GraphQL: Mutation: Create issue<br/>(title, description, label)
GraphQL-->>Client: Issue ID & Identifier
Client-->>TUI: Issue created successfully
TUI->>User: Display success message<br/>with issue Identifier
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
cmd/feedback_escape_other.go (1)
7-9: Consider documenting the fallback behavior.The function always returns
true, treating any escape key press as a bare escape (cancel). This is a safe default for unsupported platforms, but adding a brief comment would clarify the intent for future maintainers.📝 Suggested documentation
+// readEscapeSequence checks if an escape key press is a bare escape (cancel) +// or the start of an ANSI sequence. On unsupported platforms, we assume bare +// escape to fail safe. func readEscapeSequence(in *os.File) (bool, error) { return true, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/feedback_escape_other.go` around lines 7 - 9, The function readEscapeSequence currently always returns true (bare escape) as a safe fallback on unsupported platforms; add a short comment above readEscapeSequence explaining this intentional fallback behavior, why we treat any escape as a bare escape (cancel) on platforms we can't parse sequences, and note that platform-specific implementations should replace this stub (reference function name readEscapeSequence).cmd/feedback_escape_unix.go (1)
11-27: Consider adding a doc comment explaining the return value semantics.The function's purpose (distinguishing bare Esc from ANSI escape sequences) isn't immediately clear from the signature. A brief comment would help maintainers understand the intent.
📝 Suggested documentation
+// readEscapeSequence checks if an escape byte was a bare Esc key (returns true) +// or the start of an ANSI sequence like arrow keys (returns false). +// It does a non-blocking read to see if more bytes follow the initial 0x1b. func readEscapeSequence(in *os.File) (bool, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/feedback_escape_unix.go` around lines 11 - 27, Add a doc comment above the readEscapeSequence function that describes its purpose (to disambiguate a bare Esc keypress from a multi-byte ANSI escape sequence), clearly defines the boolean return semantics (true means no additional bytes were available/read and caller should treat the input as a bare Esc; false means additional bytes were read so it is an escape sequence), and notes the error return behavior (non-nil error indicates a read/SetNonblock failure). Reference the function name readEscapeSequence and the conditions that return true (unix.EAGAIN, unix.EWOULDBLOCK, or n == 0) so maintainers understand exactly when to treat the input as a bare Esc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/feedback.go`:
- Around line 126-135: The ConfigPath returned by config.ConfigFilePath() may
contain user PII; in buildFeedbackContext replace/redact the absolute path
before setting feedback.Context.ConfigPath (e.g., convert home directory to "~",
use a path relative to the home dir, or set to just the basename/default flag)
so the value sent to feedback.Context (and ultimately Linear) does not include
the full user/home path; update buildFeedbackContext to call
config.ConfigFilePath(), transform the string to a redacted form, and assign
that redacted value to ConfigPath.
- Around line 199-233: The input loop in cmd/feedback.go currently manipulates
raw bytes (buf, scratch) which corrupts multi-byte UTF-8 characters; change it
to operate on runes: read bytes from in but decode runes (using utf8.DecodeRune
or a rune reader) and store runes in a rune slice or strings.Builder, update the
backspace handler to remove the last rune instead of the last byte, and write
full runes to out (using io.WriteString or fmt.Fprint) so readEscapeSequence,
the '\r'/'\n' and control-char handling remain the same but all non-ASCII
characters are preserved correctly.
- Line 109: The CLI confirmation line uses the forbidden phrase "Container
runtime"; update the user-facing string in the EmitInfo call so it uses
"Emulator" instead (e.g., change "- Container runtime: " to "- Emulator: " or "-
Emulator type: ") while keeping the same value source (ctx.ContainerRuntime) and
preserving styles.Secondary.Render and orUnknown(ctx.ContainerRuntime) usage in
output.EmitInfo.
- Around line 34-50: Move the Linear config validation so it runs before
collectFeedbackInteractively to avoid losing user input: check
cfg.LinearClientID, cfg.LinearClientSecret, and cfg.LinearTeamID (the existing
LSTK_LINEAR_* checks) at the start of the feedback command handler and return
the fmt.Errorf errors if any are empty, then call
collectFeedbackInteractively(cmd, sink, cfg) and proceed with the
confirmed/error handling; update any related comments to reflect the new order.
In `@internal/container/start.go`:
- Line 157: The code adds a hard-coded HostPort "443" via ExtraPorts (see
ExtraPorts, servicePortRange(), httpsPort()) but preflight only validates
c.Port, so a bound 443 will cause a later runtime failure; update the startup
preflight to include and validate all ports present in ExtraPorts (including
whatever httpsPort() returns) for availability, or change httpsPort() to avoid a
fixed HostPort (use c.Port or leave HostPort unset) so it aligns with the
existing preflight checks; modify the port-validation routine to iterate
ExtraPorts and check each HostPort for conflicts before starting so startup
fails early with a clear port-conflict error.
In `@internal/feedback/client.go`:
- Around line 173-175: Change the exported error text to start with a lowercase
letter to satisfy ST1005: update the fmt.Errorf calls that build Linear API
errors (the branch checking decoded.Errors and the `"Linear API did not create
an issue"` return) so their messages begin lowercase (e.g., "linear API error:
..." and "linear API did not create an issue..." or better "linear API: did not
create an issue..."); locate and update all fmt.Errorf usages in
internal/feedback/client.go around the decoded.Errors check and the related
returns at the other noted spots (lines referenced) to use lowercase-leading
error strings.
- Around line 291-294: The truncation uses byte slicing which can split UTF-8
runes; replace the byte slice expression summary[:maxSummaryLen-3] with a
rune-safe slice (e.g., convert to []rune and slice to maxSummaryLen-3 runes,
then convert back to string) when computing the truncated summary variable
(respecting const maxSummaryLen), and add a regression test that calls the same
truncation path with CJK/emoji-heavy input to assert the result contains no
replacement characters and ends with "..." when truncated.
In `@internal/ui/run_feedback.go`:
- Around line 20-33: The background goroutine currently calling submit(...) is
started before p.Run() succeeds, risking non-idempotent Linear writes on failed
startups; instead, remove that eager goroutine and start submit only after
Bubble Tea successfully initializes by launching it from a post-start Tea
command (e.g., the model Init/Start command or a tea.Cmd returned from the
initial model) so it runs only when p.Run() returns a valid model. Move the
submit invocation and its use of runErrCh, runErrMsg, runDoneMsg and
programSender{p: p} into that post-start command, preserving the same error
handling (send runErrMsg if err != nil and not context.Canceled, otherwise send
runDoneMsg) and ensure the goroutine still writes to runErrCh to keep existing
sync semantics.
In `@test/integration/feedback_test.go`:
- Around line 82-83: The test asserts a hard-coded OS string ("darwin") but
feedback.go prints runtime.GOOS/runtime.GOARCH, so the test should be
platform-aware; update the assertions in feedback_test.go to read runtime.GOOS
and runtime.GOARCH and assert that out.String() contains the formatted OS/arch
values produced by feedback.go (e.g., use runtime.GOOS and runtime.GOARCH
variables in the expected string checks for the out.String() assertions
referencing the same output used in the current assert.Contains calls).
---
Nitpick comments:
In `@cmd/feedback_escape_other.go`:
- Around line 7-9: The function readEscapeSequence currently always returns true
(bare escape) as a safe fallback on unsupported platforms; add a short comment
above readEscapeSequence explaining this intentional fallback behavior, why we
treat any escape as a bare escape (cancel) on platforms we can't parse
sequences, and note that platform-specific implementations should replace this
stub (reference function name readEscapeSequence).
In `@cmd/feedback_escape_unix.go`:
- Around line 11-27: Add a doc comment above the readEscapeSequence function
that describes its purpose (to disambiguate a bare Esc keypress from a
multi-byte ANSI escape sequence), clearly defines the boolean return semantics
(true means no additional bytes were available/read and caller should treat the
input as a bare Esc; false means additional bytes were read so it is an escape
sequence), and notes the error return behavior (non-nil error indicates a
read/SetNonblock failure). Reference the function name readEscapeSequence and
the conditions that return true (unix.EAGAIN, unix.EWOULDBLOCK, or n == 0) so
maintainers understand exactly when to treat the input as a bare Esc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 665e8c1a-d375-46eb-98f0-bfa6e92109b1
📒 Files selected for processing (14)
CLAUDE.mdREADME.mdcmd/feedback.gocmd/feedback_escape_other.gocmd/feedback_escape_unix.gocmd/feedback_test.gocmd/root.gointernal/container/start.gointernal/env/env.gointernal/feedback/client.gointernal/feedback/client_test.gointernal/ui/run_feedback.gotest/integration/env/env.gotest/integration/feedback_test.go
| output.EmitInfo(sink, "- OS (arch): "+styles.Secondary.Render(fmt.Sprintf("%s (%s)", runtime.GOOS, runtime.GOARCH))) | ||
| output.EmitInfo(sink, "- Installation: "+styles.Secondary.Render(orUnknown(ctx.InstallMethod))) | ||
| output.EmitInfo(sink, "- Shell: "+styles.Secondary.Render(orUnknown(ctx.Shell))) | ||
| output.EmitInfo(sink, "- Container runtime: "+styles.Secondary.Render(orUnknown(ctx.ContainerRuntime))) |
There was a problem hiding this comment.
Avoid container/runtime wording in this prompt.
This is user-facing CLI copy, but the repo guidelines reserve container/runtime terminology for internal implementation details. Please rename or remove this field in the confirmation block. As per coding guidelines, "In user-facing CLI/help/docs, prefer 'emulator' over 'container'/'runtime'; use 'container'/'runtime' only for internal implementation details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/feedback.go` at line 109, The CLI confirmation line uses the forbidden
phrase "Container runtime"; update the user-facing string in the EmitInfo call
so it uses "Emulator" instead (e.g., change "- Container runtime: " to "-
Emulator: " or "- Emulator type: ") while keeping the same value source
(ctx.ContainerRuntime) and preserving styles.Secondary.Render and
orUnknown(ctx.ContainerRuntime) usage in output.EmitInfo.
gtsiolis
force-pushed
the
george/des-182
branch
from
5352cf5 to
604723a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This will add the first iteration for DES-182, that adds an interactive feedback command in the new CLI.
Each submissions ends up in our Feedback team.
Relies on https://github.com/localstack/localstack-platform/pull/1803.