Merged
Conversation
🤖 Pull request artifacts
|
WalkthroughAdds a batch SMS send subcommand (CSV/XLSX) with tabular readers, column mapping/validation, concurrent worker-based sending and reporting; refactors send flags into a reusable flags package; introduces tabular Record/Reader API and tests; bumps Go toolchain to 1.25 and updates docs and build targets. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI User
participant Cmd as Batch Send<br/>Command
participant Reader as Tabular<br/>Reader
participant Mapper as Column<br/>Mapper
participant Pool as Worker<br/>Pool
participant Gateway as SMS<br/>Gateway
participant Out as Output<br/>Handler
CLI->>Cmd: batch send --file data.csv --map phone=col1,...
Cmd->>Cmd: Validate flags, parse mapping
Cmd->>Reader: Read(path, HasHeader, Sheet/Delimiter)
Reader-->>Cmd: []Record
Cmd->>Mapper: MapAndValidateRows(records, mapping)
Mapper-->>Cmd: []SendRow, []errors
alt validate-only/dry-run
Cmd-->>Out: Report validation/dry-run output
else full send
Cmd->>Pool: Start workers (concurrency N)
loop per SendRow
Cmd->>Pool: Enqueue(SendRow)
Pool->>Gateway: Send message
Gateway-->>Pool: State, Error
Pool-->>Out: Emit per-row result
end
Out-->>CLI: Final summary (sent/failed/skipped)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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. 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: 3
🧹 Nitpick comments (6)
internal/io/tabular/csv_test.go (1)
14-45: Add a delimiter-focused regression case here.
--delimiteris part of the new batch-send surface, but this suite only exercises comma-separated input, and the no-header path does not pinRowNumber. A small semicolon/tab case plus a row-number assertion would make the reporting and mapping behavior much safer to refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/io/tabular/csv_test.go` around lines 14 - 45, Add a regression test that verifies non-comma delimiters and pins row numbering for the no-header case: create a new test (similar to TestCSVReader_Read_WithHeader/TestCSVReader_Read_WithoutHeader) that writes a semicolon- or tab-delimited CSV, constructs the reader via tabular.NewCSVReader with an explicit tab/semicolon set in tabular.CSVConfig{Path: ..., Delimiter: '\t' or ';', HasHeader: true/false}, calls reader.Read(ctx), asserts no error and expected Values mapping for named headers or generated "col_1"/"col_2" when HasHeader is false, and also asserts records[0].RowNumber equals the correct CSV row index (e.g., 2 when a header is present, 1 when no header) to lock row-number behavior.internal/io/tabular/types.go (1)
13-13:Readcurrently forces whole-file buffering.Returning
[]Recordmakes every CSV/XLSX reader materialize the full input before validation or sending can start. For the new bulk-send workflow that increases peak memory and delays cancellation/error feedback on large files; a streaming iterator/callback would scale better.Possible interface shape
type Reader interface { - Read(ctx context.Context) ([]Record, error) + ForEach(ctx context.Context, fn func(Record) error) error }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/io/tabular/types.go` at line 13, The Read method in internal/io/tabular/types.go currently returns []Record which forces full-file buffering; change the Read signature to stream records (e.g., Read(ctx context.Context, handler func(Record) error) error or Read(ctx context.Context) (<-chan Record, <-chan error)) so readers emit records as they parse them, honor ctx cancellation, and return errors immediately; update every implementation of Read and all callers to use the new streaming callback/iterator pattern, ensure Record remains the emitted type, and propagate parsing/IO errors and context cancellation without accumulating all records in memory.internal/commands/messages/send.go (1)
197-210: Keep the data path out ofnewTextMessageRequest.This block now constructs every request through the text-message helper and then rewrites
DataMessage/TextMessageafterward. That couples data sends to text-only defaults frominternal/commands/messages/send_request.go:15-29and also allocates/discards a duplicateTextMessageon the text path. Prefer a neutral base-request builder, or only call the helper in the non-data branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/send.go` around lines 197 - 210, The current code always calls newTextMessageRequest(messageRequestInput{...}) and then overwrites DataMessage/TextMessage, which couples data sends to text defaults and allocates a duplicate TextMessage; change this to build a neutral base request (e.g., construct messageRequestInput directly into a new generic request) or only call newTextMessageRequest when sending a text message: detect whether dataMessage != nil and if so create the request from a neutral builder using messageRequestInput fields and assign DataMessage, TTL, ValidUntil, etc., otherwise call newTextMessageRequest(...) and set the remaining fields; target the newTextMessageRequest call and the assignment of DataMessage/TextMessage in send.go to eliminate the redundant allocation and coupling.internal/io/tabular/xlsx_test.go (1)
14-40: Test covers the happy path well, but consider adding edge case coverage.The test validates header-mapped reading correctly. Consider adding tests for:
HasHeader: false(column keys should becol_1,col_2, etc.)- Empty sheet or missing sheet name
- Multiple data rows
- Rows with fewer columns than headers (sparse data)
These would mirror the CSV test patterns and strengthen confidence in the XLSX reader.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/io/tabular/xlsx_test.go` around lines 14 - 40, Add additional unit tests alongside TestXLSXReader_Read_WithHeader to cover edge cases: create TestXLSXReader_Read_NoHeader that uses tabular.NewXLSXReader with XLSXConfig{HasHeader:false} and assert keys are "col_1", "col_2", etc.; create TestXLSXReader_Read_EmptySheet which calls reader.Read on an empty sheet and asserts zero records (and another test for missing sheet name to assert an error); create TestXLSXReader_Read_MultipleRows that writes multiple data rows and asserts correct RowNumber and Values for each record; and create TestXLSXReader_Read_SparseRows that writes rows with fewer columns than the header and assert missing keys are absent or empty as per reader.Read behavior. Use the same pattern as the existing test (excelize.NewFile, SetCellValue, SaveAs, tabular.NewXLSXReader, reader.Read(context.Background()), and require/assert checks) and mirror the CSV test naming/assertions for consistency.internal/io/tabular/csv.go (1)
45-48: Consider streaming for large files.
ReadAll()loads the entire CSV into memory. For large input files (the design doc mentions 10k rows as a test target), this could consume significant memory. For now this is acceptable, but consider streaming row-by-row in a future iteration if memory becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/io/tabular/csv.go` around lines 45 - 48, The current use of reader.ReadAll() (producing rows) loads the entire CSV into memory which can be problematic for large files; change the implementation that calls reader.ReadAll() to instead iterate row-by-row using csv.Reader.Read() (or a loop with reader.Read() until io.EOF), processing or appending each record incrementally so you never hold the entire file in memory; update the code paths that consume the rows variable to work with streaming (e.g., process each record in the loop or send to a channel/consumer), and ensure you handle io.EOF and other read errors correctly while preserving existing error wrapping semantics.internal/commands/messages/batch_send.go (1)
459-469: JSON report writer doesn't sync file before close.Consider calling
file.Sync()before close to ensure data is flushed to disk, especially for operator-critical reports.🛡️ Proposed fix
func writeBatchReportJSON(path string, report []batchReportEntry) error { file, err := os.Create(path) if err != nil { return err } defer file.Close() encoder := json.NewEncoder(file) encoder.SetIndent("", " ") - return encoder.Encode(report) + if err := encoder.Encode(report); err != nil { + return err + } + return file.Sync() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch_send.go` around lines 459 - 469, The JSON report writer writeBatchReportJSON should explicitly flush to disk before closing: after calling encoder.Encode(report) check its error, then call file.Sync() and handle/return any Sync error (preferably returning the Sync error if non-nil or combining it with the encode error), and only then allow the deferred file.Close() to run; update writeBatchReportJSON to perform this Sync step to ensure on-disk durability for operator-critical reports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs_batch_sending_plan.md`:
- Line 11: The heading "## Option A — Extend existing `smsgate send` command
(recommended)" is inconsistent with the document's final recommendation of
Option B; remove the literal " (recommended)" suffix from that heading so it
reads "## Option A — Extend existing `smsgate send` command" and verify no other
headings or inline labels still mark Option A as recommended (update any
matching occurrences of the phrase "(recommended)" tied to Option A).
In `@go.mod`:
- Line 25: The go.mod entry for module golang.org/x/crypto must be upgraded from
v0.28.0 to v0.45.0 (or later) to resolve multiple security advisories; update
the version in go.mod (the golang.org/x/crypto module line), then run go get
golang.org/x/crypto@v0.45.0 and go mod tidy (or go mod vendor if vendoring) to
update the lockfile and dependencies, and run the test suite/build to ensure
nothing breaks.
In `@internal/commands/messages/batch_send.go`:
- Line 227: Add a package-level sentinel error block after the imports (var
ErrUnsupportedExtension = errors.New("unsupported file extension"), ErrMapEmpty,
ErrInvalidMapping, ErrPhoneEmpty, ErrTextEmpty, ErrInvalidPriority) and replace
all dynamic fmt.Errorf calls that create those messages in
internal/commands/messages/batch_send.go with wrapped sentinel errors (e.g.
fmt.Errorf("%w: %q; use .csv or .xlsx", ErrUnsupportedExtension, ext)). Update
each return site that currently returns dynamic errors (the sites flagged in the
review) to use the appropriate sentinel (ErrMapEmpty, ErrInvalidMapping,
ErrPhoneEmpty, ErrTextEmpty, ErrInvalidPriority) wrapped with context via
fmt.Errorf("%w: <context>", ErrX) so the linter no longer sees dynamic error
creation while preserving message detail.
---
Nitpick comments:
In `@internal/commands/messages/batch_send.go`:
- Around line 459-469: The JSON report writer writeBatchReportJSON should
explicitly flush to disk before closing: after calling encoder.Encode(report)
check its error, then call file.Sync() and handle/return any Sync error
(preferably returning the Sync error if non-nil or combining it with the encode
error), and only then allow the deferred file.Close() to run; update
writeBatchReportJSON to perform this Sync step to ensure on-disk durability for
operator-critical reports.
In `@internal/commands/messages/send.go`:
- Around line 197-210: The current code always calls
newTextMessageRequest(messageRequestInput{...}) and then overwrites
DataMessage/TextMessage, which couples data sends to text defaults and allocates
a duplicate TextMessage; change this to build a neutral base request (e.g.,
construct messageRequestInput directly into a new generic request) or only call
newTextMessageRequest when sending a text message: detect whether dataMessage !=
nil and if so create the request from a neutral builder using
messageRequestInput fields and assign DataMessage, TTL, ValidUntil, etc.,
otherwise call newTextMessageRequest(...) and set the remaining fields; target
the newTextMessageRequest call and the assignment of DataMessage/TextMessage in
send.go to eliminate the redundant allocation and coupling.
In `@internal/io/tabular/csv_test.go`:
- Around line 14-45: Add a regression test that verifies non-comma delimiters
and pins row numbering for the no-header case: create a new test (similar to
TestCSVReader_Read_WithHeader/TestCSVReader_Read_WithoutHeader) that writes a
semicolon- or tab-delimited CSV, constructs the reader via tabular.NewCSVReader
with an explicit tab/semicolon set in tabular.CSVConfig{Path: ..., Delimiter:
'\t' or ';', HasHeader: true/false}, calls reader.Read(ctx), asserts no error
and expected Values mapping for named headers or generated "col_1"/"col_2" when
HasHeader is false, and also asserts records[0].RowNumber equals the correct CSV
row index (e.g., 2 when a header is present, 1 when no header) to lock
row-number behavior.
In `@internal/io/tabular/csv.go`:
- Around line 45-48: The current use of reader.ReadAll() (producing rows) loads
the entire CSV into memory which can be problematic for large files; change the
implementation that calls reader.ReadAll() to instead iterate row-by-row using
csv.Reader.Read() (or a loop with reader.Read() until io.EOF), processing or
appending each record incrementally so you never hold the entire file in memory;
update the code paths that consume the rows variable to work with streaming
(e.g., process each record in the loop or send to a channel/consumer), and
ensure you handle io.EOF and other read errors correctly while preserving
existing error wrapping semantics.
In `@internal/io/tabular/types.go`:
- Line 13: The Read method in internal/io/tabular/types.go currently returns
[]Record which forces full-file buffering; change the Read signature to stream
records (e.g., Read(ctx context.Context, handler func(Record) error) error or
Read(ctx context.Context) (<-chan Record, <-chan error)) so readers emit records
as they parse them, honor ctx cancellation, and return errors immediately;
update every implementation of Read and all callers to use the new streaming
callback/iterator pattern, ensure Record remains the emitted type, and propagate
parsing/IO errors and context cancellation without accumulating all records in
memory.
In `@internal/io/tabular/xlsx_test.go`:
- Around line 14-40: Add additional unit tests alongside
TestXLSXReader_Read_WithHeader to cover edge cases: create
TestXLSXReader_Read_NoHeader that uses tabular.NewXLSXReader with
XLSXConfig{HasHeader:false} and assert keys are "col_1", "col_2", etc.; create
TestXLSXReader_Read_EmptySheet which calls reader.Read on an empty sheet and
asserts zero records (and another test for missing sheet name to assert an
error); create TestXLSXReader_Read_MultipleRows that writes multiple data rows
and asserts correct RowNumber and Values for each record; and create
TestXLSXReader_Read_SparseRows that writes rows with fewer columns than the
header and assert missing keys are absent or empty as per reader.Read behavior.
Use the same pattern as the existing test (excelize.NewFile, SetCellValue,
SaveAs, tabular.NewXLSXReader, reader.Read(context.Background()), and
require/assert checks) and mirror the CSV test naming/assertions for
consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6e909f6-788e-4bc6-8694-c064c3aec24d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
docs_batch_sending_plan.mdgo.modinternal/commands/messages/batch.gointernal/commands/messages/batch_send.gointernal/commands/messages/batch_send_test.gointernal/commands/messages/batch_send_types.gointernal/commands/messages/messages.gointernal/commands/messages/send.gointernal/commands/messages/send_request.gointernal/io/tabular/csv.gointernal/io/tabular/csv_test.gointernal/io/tabular/types.gointernal/io/tabular/xlsx.gointernal/io/tabular/xlsx_test.go
capcom6
force-pushed
the
codex/plan-message-sending-implementation-from-csv
branch
from
35bac96 to
a6f853f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/io/tabular/csv_test.go (1)
14-45: Add a regression test for colliding normalized header names.Please add a case like headers
a,a,a_2and assert unique output keys, so future changes don’t reintroduce silent key collisions inRecord.Values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/io/tabular/csv_test.go` around lines 14 - 45, Add a regression test to csv_test.go that verifies header normalization avoids silent key collisions: create a new test (e.g., TestCSVReader_Read_HeaderCollision) that writes a CSV with headers "a,a,a_2" and one data row, use tabular.NewCSVReader with CSVConfig{Path: path, HasHeader: true}, call reader.Read(ctx) and assert no error, then assert Record.Values contains unique keys for each original header (i.e., normalized/renamed keys do not collide) and that each key maps to the correct cell value; reference the existing test patterns in TestCSVReader_Read_WithHeader to follow setup and assertions and use the same helpers (require, assert) to validate uniqueness and value mapping.internal/commands/messages/batch/batch_send.go (1)
385-385: HardcodedWithDeliveryReport: truemay not match user expectations.The delivery report flag is unconditionally set to
true. Consider either documenting this behavior or adding a--with-delivery-reportflag for user control, similar to the single-messagesendcommand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send.go` at line 385, The code unconditionally sets WithDeliveryReport to true; add a CLI flag (e.g., --with-delivery-report) on the batch send command and use that flag value instead of the hardcoded true: introduce a boolean variable (e.g., withDeliveryReport bool) in the command setup, register it with the command flags and help text (mirroring the single-message send command), and replace WithDeliveryReport: lo.ToPtr(true) with WithDeliveryReport: lo.ToPtr(withDeliveryReport) in the function that builds the message payload (in batch_send.go).
🤖 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/commands/messages/batch/batch_send.go`:
- Around line 111-112: In batchSendAction, guard against nil returns from
metadata.GetRenderer and metadata.GetClient: after calling
metadata.GetRenderer(c.App.Metadata) and metadata.GetClient(c.App.Metadata)
check if renderer or client is nil and return a descriptive error (or cli.Exit)
instead of continuing; update any later code that dereferences renderer or
client (e.g., calls on renderer or client) to only run when the non-nil checks
pass so the function does not panic.
- Around line 101-106: The Before hook in batchSendCmd checks c.String("report")
and validates its extension (using c.String("report") and filepath.Ext) but the
batchSendCmd() command does not define a "report" flag, so this validation is
dead; either add a StringFlag named "report" to batchSendCmd() (e.g., a
&cli.StringFlag{Name: "report", Usage: "Path to output report file (.csv or
.json)", Value: ""}) so c.String("report") can return the user value, or remove
the validation block from the Before hook if the feature is not implemented yet;
update the code around batchSendCmd and the Before hook accordingly to keep flag
names consistent.
In `@pkg/io/tabular/csv.go`:
- Around line 91-107: normalizeHeaders can still emit duplicates because it
increments counts based only on the base token in seen rather than the final
emitted key; update the algorithm in normalizeHeaders to track already-emitted
final keys and ensure uniqueness by looping: after trimming and defaulting empty
names to "col_<i+1>", attempt the candidate key and if it already exists in a
map of emitted keys, increment a numeric suffix (e.g., "_2", "_3", ...) until a
unique finalKey is found, then record finalKey in the emitted map and append it
to headers (also maintain any counters needed for faster next-suffix
generation); this ensures cases like "a,a,a_2" produce unique output and
prevents overwriting in Record.Values.
In `@pkg/io/tabular/xlsx.go`:
- Around line 45-48: The rows iterator returned by f.Rows(sheet) is not closed,
causing a resource leak; after successfully obtaining rows (the rows variable
from f.Rows), immediately schedule rows.Close() (e.g., defer rows.Close()) so
the iterator is always closed even on early returns or errors during iteration;
update the function containing the f.Rows call to call rows.Close() via defer
right after the nil-check on err to ensure temporary XML resources are released.
---
Nitpick comments:
In `@internal/commands/messages/batch/batch_send.go`:
- Line 385: The code unconditionally sets WithDeliveryReport to true; add a CLI
flag (e.g., --with-delivery-report) on the batch send command and use that flag
value instead of the hardcoded true: introduce a boolean variable (e.g.,
withDeliveryReport bool) in the command setup, register it with the command
flags and help text (mirroring the single-message send command), and replace
WithDeliveryReport: lo.ToPtr(true) with WithDeliveryReport:
lo.ToPtr(withDeliveryReport) in the function that builds the message payload (in
batch_send.go).
In `@pkg/io/tabular/csv_test.go`:
- Around line 14-45: Add a regression test to csv_test.go that verifies header
normalization avoids silent key collisions: create a new test (e.g.,
TestCSVReader_Read_HeaderCollision) that writes a CSV with headers "a,a,a_2" and
one data row, use tabular.NewCSVReader with CSVConfig{Path: path, HasHeader:
true}, call reader.Read(ctx) and assert no error, then assert Record.Values
contains unique keys for each original header (i.e., normalized/renamed keys do
not collide) and that each key maps to the correct cell value; reference the
existing test patterns in TestCSVReader_Read_WithHeader to follow setup and
assertions and use the same helpers (require, assert) to validate uniqueness and
value mapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ff9f6eb-69d8-4347-99c5-ceaa24c2a329
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
README.mdgo.modinternal/commands/messages/batch/batch.gointernal/commands/messages/batch/batch_send.gointernal/commands/messages/batch/batch_send_test.gointernal/commands/messages/batch/batch_send_types.gointernal/commands/messages/batch/errors.gointernal/commands/messages/batch/mappings.gointernal/commands/messages/messages.gopkg/io/tabular/csv.gopkg/io/tabular/csv_test.gopkg/io/tabular/types.gopkg/io/tabular/xlsx.gopkg/io/tabular/xlsx_test.gouser-docs
✅ Files skipped from review due to trivial changes (2)
- user-docs
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/commands/messages/messages.go
- go.mod
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/commands/messages/send.go (1)
100-100: Discarded error fromNewSendFlagscould mask validation failures.The error from
flags.NewSendFlags(c)is discarded with_. WhilesendBeforealready validates the flags, re-parsing here could fail if:
- Time elapsed causes
ValidUntilto now be in the past- Any other time-sensitive check fails
Consider storing the validated
SendFlagsincli.Context.App.MetadataduringBeforeand retrieving it inActionto avoid redundant parsing and potential inconsistencies.♻️ Alternative approach
// In sendBefore: sendFlags, err := flags.NewSendFlags(c) if err != nil { return cli.Exit(err.Error(), codes.ParamsError) } c.App.Metadata["sendFlags"] = sendFlags // In sendAction: sendFlags := c.App.Metadata["sendFlags"].(*flags.SendFlags)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/send.go` at line 100, Discarding the error from flags.NewSendFlags(c) in sendAction can mask validation failures and cause time-sensitive inconsistencies; instead, parse and validate once in sendBefore by calling flags.NewSendFlags(c), handle/return the error there, and store the resulting *flags.SendFlags in c.App.Metadata (e.g., c.App.Metadata["sendFlags"] = sendFlags); then in sendAction retrieve and type-assert the stored sendFlags from c.App.Metadata (e.g., sendFlags := c.App.Metadata["sendFlags"].(*flags.SendFlags)) so you avoid re-parsing and potential missed errors between sendBefore and sendAction.internal/commands/flags/send.go (1)
117-121: Time-sensitive validation may produce inconsistent results when called multiple times.The
ValidUntilcheck at line 118 validates againsttime.Now(). This function is called twice in thesendcommand flow: once insendBefore(where errors are handled) and again insendAction(where the error is discarded with_). If avalid-untiltimestamp is very close to the current time, it could pass validation inBeforebut technically be in the past by the timeActionruns.Consider caching the validated
SendFlagsin thecli.ContextduringBeforeso it can be reused inActionwithout re-parsing and re-validating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/flags/send.go` around lines 117 - 121, The ValidUntil check currently calls time.Now() twice (in sendBefore and again in sendAction) which can cause racey failures for timestamps near the present; update sendBefore to parse and validate the flags once (the SendFlags.ValidUntil value) and store the validated SendFlags object into the cli.Context (under a stable key) so sendAction retrieves that cached struct instead of re-calling c.Timestamp/validating; ensure sendAction falls back to the cached SendFlags and does not re-validate ValidUntil (remove the duplicate time.Now() check there) so both phases use the exact same validated timestamp.internal/commands/messages/batch/batch_send.go (1)
125-125: Discarded error fromNewSendFlagsmirrors the issue insend.go.Same as noted in
send.go, the error fromflags.NewSendFlags(c)is discarded. Consider storing validated flags duringbatchSendBeforefor reuse inbatchSendAction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send.go` at line 125, The code discards the error from flags.NewSendFlags(c) in batch_send.go; change it to capture and handle the error in batchSendBefore (call sendFlags, err := flags.NewSendFlags(c)), return the error if non-nil, and store the validated sendFlags for reuse in batchSendAction (e.g., attach to the command/context metadata or add a field on the batch send command struct). Update batchSendAction to retrieve and use the stored sendFlags instead of creating them again.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 273-275: Update the README column mapping to reflect the actual
priority bounds used by the code: replace the "0-255" range for `priority` with
the signed int8 range used by the implementation (use the constants
`smsgateway.PriorityMinimum` to `smsgateway.PriorityMaximum`, e.g. -128 to 127)
so the table matches the `send` command docs and the validation in
`flags/send.go`; ensure the README text mentions the same constants or numeric
range to avoid future drift.
---
Nitpick comments:
In `@internal/commands/flags/send.go`:
- Around line 117-121: The ValidUntil check currently calls time.Now() twice (in
sendBefore and again in sendAction) which can cause racey failures for
timestamps near the present; update sendBefore to parse and validate the flags
once (the SendFlags.ValidUntil value) and store the validated SendFlags object
into the cli.Context (under a stable key) so sendAction retrieves that cached
struct instead of re-calling c.Timestamp/validating; ensure sendAction falls
back to the cached SendFlags and does not re-validate ValidUntil (remove the
duplicate time.Now() check there) so both phases use the exact same validated
timestamp.
In `@internal/commands/messages/batch/batch_send.go`:
- Line 125: The code discards the error from flags.NewSendFlags(c) in
batch_send.go; change it to capture and handle the error in batchSendBefore
(call sendFlags, err := flags.NewSendFlags(c)), return the error if non-nil, and
store the validated sendFlags for reuse in batchSendAction (e.g., attach to the
command/context metadata or add a field on the batch send command struct).
Update batchSendAction to retrieve and use the stored sendFlags instead of
creating them again.
In `@internal/commands/messages/send.go`:
- Line 100: Discarding the error from flags.NewSendFlags(c) in sendAction can
mask validation failures and cause time-sensitive inconsistencies; instead,
parse and validate once in sendBefore by calling flags.NewSendFlags(c),
handle/return the error there, and store the resulting *flags.SendFlags in
c.App.Metadata (e.g., c.App.Metadata["sendFlags"] = sendFlags); then in
sendAction retrieve and type-assert the stored sendFlags from c.App.Metadata
(e.g., sendFlags := c.App.Metadata["sendFlags"].(*flags.SendFlags)) so you avoid
re-parsing and potential missed errors between sendBefore and sendAction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33c1e5f3-21f5-41e3-91c4-49b25e0b368d
📒 Files selected for processing (7)
README.mdinternal/commands/flags/errors.gointernal/commands/flags/send.gointernal/commands/messages/batch/batch_send.gointernal/commands/messages/send.gopkg/io/tabular/csv.gopkg/io/tabular/xlsx.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 285-286: Clarify that for headerless CSV/XLSX files column names
are autogenerated: update the README example line that currently reads "No
headers, column positions" (the example using `smsgate batch send --header=false
--map phone=col_1,text=col_2 data.csv`) to state something like "No headers —
column names auto-generated as col_1, col_2, ... (1-indexed)" so users know the
synthetic naming convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a563b74-df1f-41d1-a498-212db082f664
📒 Files selected for processing (3)
MakefileREADME.mdtests/e2e/messages_test.go
capcom6
force-pushed
the
codex/plan-message-sending-implementation-from-csv
branch
from
b3734e5 to
7e102ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/commands/flags/send.go`:
- Around line 111-115: The TTL handling currently truncates fractional seconds
by converting ttl.Seconds() (float64) to uint64; update the validation in the
code around c.Duration("ttl") so that after checking ttl is non-negative you
also reject any fractional-second values (e.g. if ttl%time.Second != 0) and
return ErrValidationFailed with a clear message; only after this integer-second
check set fl.TTL = lo.EmptyableToPtr(uint64(ttl.Seconds())). Reference symbols:
ttl variable, c.Duration, fl.TTL, lo.EmptyableToPtr, ErrValidationFailed.
In `@internal/commands/messages/batch/batch_send.go`:
- Around line 305-314: The sim_number parsing accepts 0 and sets row.SimNumber
to &uint8(0); change the validation in the block handling mapping["sim_number"]
(where strconv.ParseUint is used and row.SimNumber is assigned) to reject a
parsed value of 0 by returning an error (e.g., "invalid sim_number: must be
1..255") before converting to uint8 and assigning the pointer; ensure the error
path uses the same sendRow error flow so malformed rows are rejected during
validation rather than later in the send flow.
- Around line 392-409: The row-specific priority is being collapsed to 0 during
initial req construction (Priority:
smsgateway.MessagePriority(lo.FromPtrOr(row.Priority, 0))) so sendFlags.Merge
can overwrite it; instead build the base req with no row priority (leave
Priority nil), call sendFlags.Merge(req) to apply shared flags, then apply the
row override only if row.Priority != nil by setting req.Priority =
smsgateway.MessagePriority(*row.Priority); update the code paths around the req
variable, sendFlags.Merge, and the use of row.Priority to ensure an explicit 0
from the row is preserved.
- Around line 255-261: parseColumnMapping currently accepts any mapping key but
mapRow only uses a fixed set, so unknown keys (e.g., "device-id") are silently
ignored; update parseColumnMapping to validate each key against the canonical
allowed set used by mapRow (e.g., "device", "sim", "routing", "priority" — use
the exact identifiers mapRow expects) and return an error (wrap
ErrMappingParseFailed) when an unknown key is encountered; ensure you reference
the same allowed-key list used by mapRow (or centralize it) so validation and
usage stay in sync.
In `@README.md`:
- Around line 229-246: The docs incorrectly state that `batch send` inherits all
`send` options; update README to clarify that `batch send` only reuses the
shared delivery/device flags produced by flags.Send() (not the full set defined
in internal/commands/messages/send.go such as `--id`, `--phones`, `--data`,
`--data-port`), and adjust examples accordingly; also correct the mapping
guidance to explain that headerless files generate synthetic column names
(`col_1`, `col_2`, ...) so mappings like `--map phone=1,text=2` won’t resolve —
show the proper use of `--map phone=col_1,text=col_2` or include a header, and
apply the same corrections to the other referenced lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fe58a20-4416-4032-a26b-11b34284951f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
MakefileREADME.mdgo.modinternal/commands/flags/errors.gointernal/commands/flags/send.gointernal/commands/messages/batch/batch.gointernal/commands/messages/batch/batch_send.gointernal/commands/messages/batch/batch_send_test.gointernal/commands/messages/batch/batch_send_types.gointernal/commands/messages/batch/errors.gointernal/commands/messages/batch/mappings.gointernal/commands/messages/messages.gointernal/commands/messages/send.gopkg/io/tabular/csv.gopkg/io/tabular/csv_test.gopkg/io/tabular/types.gopkg/io/tabular/xlsx.gopkg/io/tabular/xlsx_test.gotests/e2e/messages_test.gouser-docs
✅ Files skipped from review due to trivial changes (10)
- internal/commands/messages/messages.go
- internal/commands/messages/batch/batch_send_types.go
- user-docs
- internal/commands/flags/errors.go
- internal/commands/messages/batch/errors.go
- pkg/io/tabular/types.go
- pkg/io/tabular/xlsx_test.go
- pkg/io/tabular/csv_test.go
- go.mod
- pkg/io/tabular/xlsx.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/commands/messages/batch/batch.go
- tests/e2e/messages_test.go
- Makefile
- internal/commands/messages/batch/mappings.go
- internal/commands/messages/batch/batch_send_test.go
capcom6
force-pushed
the
codex/plan-message-sending-implementation-from-csv
branch
from
2d93e61 to
9cdd391
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/io/tabular/xlsx_test.go (1)
14-40: LGTM! Test correctly validates XLSX reading with headers.The test structure is sound: parallel execution, automatic temp dir cleanup, proper use of
requirefor setup steps andassertfor value checks. TheRowNumber == 2assertion correctly reflects 1-based Excel row numbering when the header occupies row 1.Consider adding a companion
TestXLSXReader_Read_WithoutHeadertest to match the CSV reader test coverage mentioned in the PR description (header/no-header parsing). This would verify theHasHeader: falsecode path where column indices become keys.♻️ Suggested test for no-header scenario
func TestXLSXReader_Read_WithoutHeader(t *testing.T) { t.Parallel() dir := t.TempDir() path := filepath.Join(dir, "input.xlsx") f := excelize.NewFile() defer f.Close() index, err := f.NewSheet("Data") require.NoError(t, err) f.SetActiveSheet(index) require.NoError(t, f.SetCellValue("Data", "A1", "+12025550123")) require.NoError(t, f.SetCellValue("Data", "B1", "Hello")) require.NoError(t, f.SaveAs(path)) reader := tabular.NewXLSXReader(tabular.XLSXConfig{Path: path, Sheet: "Data", HasHeader: false}) records, err := reader.Read(context.Background()) require.NoError(t, err) require.Len(t, records, 1) assert.Equal(t, 1, records[0].RowNumber) assert.Equal(t, "+12025550123", records[0].Values["0"]) assert.Equal(t, "Hello", records[0].Values["1"]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/io/tabular/xlsx_test.go` around lines 14 - 40, Add a complementary test to cover the no-header code path by creating TestXLSXReader_Read_WithoutHeader that mirrors TestXLSXReader_Read_WithHeader but sets tabular.XLSXConfig{HasHeader: false}; write the first row as data (not header), call reader.Read(context.Background()), assert one record, assert records[0].RowNumber equals 1, and assert fields are keyed by column index strings ("0", "1") with the expected values; ensure you reuse the same excelize.NewFile setup and temp file pattern so the test is parallel-safe and cleans up.internal/commands/flags/send.go (1)
120-137: Consider including invalid values in error messages for debugging.The SIM number validation error helpfully includes the actual invalid value, but the ValidUntil and Priority errors don't. Including the actual values aids debugging.
♻️ Suggested improvement
validUntil := c.Timestamp("valid-until") if validUntil != nil && validUntil.Before(time.Now()) { - return nil, fmt.Errorf("%w: Valid Until must be in the future", ErrValidationFailed) + return nil, fmt.Errorf("%w: Valid Until must be in the future: %s", ErrValidationFailed, validUntil.Format(time.RFC3339)) } fl.ValidUntil = validUntil priority := c.Int( "priority", ) if priority < int(smsgateway.PriorityMinimum) || priority > int(smsgateway.PriorityMaximum) { return nil, fmt.Errorf( - "%w: Priority must be between %d and %d", + "%w: Priority must be between %d and %d: %d", ErrValidationFailed, smsgateway.PriorityMinimum, smsgateway.PriorityMaximum, + priority, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/flags/send.go` around lines 120 - 137, The ValidUntil and Priority validation errors lack the invalid values; update the validation error returns in the parsing function that sets fl.ValidUntil and priority to include the actual invalid values: when validUntil != nil && validUntil.Before(time.Now()) return ErrValidationFailed with the invalid validUntil value formatted (e.g., RFC3339) alongside the message, and when priority is out of range return ErrValidationFailed including the provided priority integer as well as the allowed min/max (smsgateway.PriorityMinimum and smsgateway.PriorityMaximum) so logs show the offending input for easier debugging.README.md (1)
250-253: Minor: Consider simplifying "XLSX sheet name".The static analysis tool notes that "XLSX" contains "spreadsheet", making "XLSX sheet" technically redundant. Consider simplifying to "Sheet name" since the context makes the file type clear.
Option Description --sheetSheet name (defaults to first sheet) 🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 250 - 253, Update the README table entry for the `--sheet` option to remove the redundant "XLSX" wording and simplify the Description to "Sheet name (defaults to first sheet)"; locate the `--sheet` row in the options table and replace "XLSX sheet name (defaults to first sheet)" with the simplified text so the context remains clear and concise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/commands/flags/send.go`:
- Around line 120-137: The ValidUntil and Priority validation errors lack the
invalid values; update the validation error returns in the parsing function that
sets fl.ValidUntil and priority to include the actual invalid values: when
validUntil != nil && validUntil.Before(time.Now()) return ErrValidationFailed
with the invalid validUntil value formatted (e.g., RFC3339) alongside the
message, and when priority is out of range return ErrValidationFailed including
the provided priority integer as well as the allowed min/max
(smsgateway.PriorityMinimum and smsgateway.PriorityMaximum) so logs show the
offending input for easier debugging.
In `@pkg/io/tabular/xlsx_test.go`:
- Around line 14-40: Add a complementary test to cover the no-header code path
by creating TestXLSXReader_Read_WithoutHeader that mirrors
TestXLSXReader_Read_WithHeader but sets tabular.XLSXConfig{HasHeader: false};
write the first row as data (not header), call
reader.Read(context.Background()), assert one record, assert
records[0].RowNumber equals 1, and assert fields are keyed by column index
strings ("0", "1") with the expected values; ensure you reuse the same
excelize.NewFile setup and temp file pattern so the test is parallel-safe and
cleans up.
In `@README.md`:
- Around line 250-253: Update the README table entry for the `--sheet` option to
remove the redundant "XLSX" wording and simplify the Description to "Sheet name
(defaults to first sheet)"; locate the `--sheet` row in the options table and
replace "XLSX sheet name (defaults to first sheet)" with the simplified text so
the context remains clear and concise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6ecb980-4f5e-41d7-9f93-356cd33920f3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
MakefileREADME.mdgo.modinternal/commands/flags/errors.gointernal/commands/flags/send.gointernal/commands/messages/batch/batch.gointernal/commands/messages/batch/batch_send.gointernal/commands/messages/batch/batch_send_test.gointernal/commands/messages/batch/batch_send_types.gointernal/commands/messages/batch/errors.gointernal/commands/messages/batch/mappings.gointernal/commands/messages/messages.gointernal/commands/messages/send.gopkg/io/tabular/csv.gopkg/io/tabular/csv_test.gopkg/io/tabular/types.gopkg/io/tabular/xlsx.gopkg/io/tabular/xlsx_test.gotests/e2e/messages_test.gouser-docs
✅ Files skipped from review due to trivial changes (7)
- user-docs
- internal/commands/flags/errors.go
- pkg/io/tabular/types.go
- internal/commands/messages/batch/errors.go
- pkg/io/tabular/csv_test.go
- internal/commands/messages/send.go
- internal/commands/messages/batch/batch_send_types.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/commands/messages/batch/batch.go
- internal/commands/messages/messages.go
- tests/e2e/messages_test.go
- internal/commands/messages/batch/mappings.go
- Makefile
- go.mod
- pkg/io/tabular/xlsx.go
- internal/commands/messages/batch/batch_send_test.go
- pkg/io/tabular/csv.go
- internal/commands/messages/batch/batch_send.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/io/tabular/xlsx_test.go (1)
42-66: LGTM!Correctly validates the no-header path where column keys are auto-generated as
col_1,col_2(1-indexed) andRowNumberstarts at 1, matching the implementation.Consider expanding test coverage in a follow-up to include edge cases such as:
- Multiple data rows
- Empty cells or rows
- Error cases (missing file, invalid sheet name)
- Verifying that only expected keys exist in
Valuesmap (to catch unintended extra columns)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/io/tabular/xlsx_test.go` around lines 42 - 66, Add additional test cases for XLSX reading edge cases: create new tests alongside TestXLSXReader_Read_WithoutHeader that use tabular.NewXLSXReader with tabular.XLSXConfig to verify multiple data rows, empty cells/rows, error scenarios (missing file, invalid sheet name), and assert that each record's RowNumber and Values map contain only the expected keys (e.g., "col_1","col_2", etc.) to catch unintended extra columns; reuse the excelize file setup pattern from TestXLSXReader_Read_WithoutHeader and assert errors with require.Error / require.NoError as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/io/tabular/xlsx_test.go`:
- Around line 42-66: Add additional test cases for XLSX reading edge cases:
create new tests alongside TestXLSXReader_Read_WithoutHeader that use
tabular.NewXLSXReader with tabular.XLSXConfig to verify multiple data rows,
empty cells/rows, error scenarios (missing file, invalid sheet name), and assert
that each record's RowNumber and Values map contain only the expected keys
(e.g., "col_1","col_2", etc.) to catch unintended extra columns; reuse the
excelize file setup pattern from TestXLSXReader_Read_WithoutHeader and assert
errors with require.Error / require.NoError as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 540407c9-0d97-4fd3-82e4-e8289a6f4696
📒 Files selected for processing (2)
internal/commands/flags/send.gopkg/io/tabular/xlsx_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/commands/flags/send.go
capcom6
force-pushed
the
codex/plan-message-sending-implementation-from-csv
branch
from
af49f7f to
90062e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
internal/commands/messages/batch/batch_send.go (3)
315-324:⚠️ Potential issue | 🟠 MajorReject
sim_number=0during row validation.The batch path still accepts
0, even though the CLI/docs describe SIM slots as one-based. That lets malformed rows pass validation and fail only after sending starts.🔧 Suggested fix
if column, ok := mapping["sim_number"]; ok { raw := strings.TrimSpace(record.Values[column]) if raw != "" { sim, err := strconv.ParseUint(raw, 10, 8) if err != nil { return sendRow{}, fmt.Errorf("invalid sim_number: %w", err) } + if sim == 0 { + return sendRow{}, fmt.Errorf("%w: sim_number must be between 1 and 255", ErrValidationFailed) + } sim8 := uint8(sim) row.SimNumber = &sim8 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send.go` around lines 315 - 324, The sim_number parsing in the batch row validation (inside the block checking mapping["sim_number"]) currently accepts 0; update the logic in this parsing branch (where strconv.ParseUint is used and row.SimNumber is set) to treat 0 as invalid: after parsing the uint value, if sim == 0 return an error (e.g., "invalid sim_number: must be 1-255") instead of assigning it to sendRow.SimNumber; ensure the error is returned from the same function so rows with sim_number=0 are rejected during validation.
23-75:⚠️ Potential issue | 🟠 MajorThe report output for this workflow still isn’t wired in.
The PR objective calls out CSV/JSON batch reports, but this command does not expose a
--reportflag and the action path never writes a report artifact. As written, the reporting part of the feature is unreachable.If helpful, I can outline a minimal
--reportflag + writer flow that works for both dry-run and full-send modes.Also applies to: 147-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send.go` around lines 23 - 75, The command flags slice fl in batch_send.go lacks a --report option and the send path never creates or writes a report artifact; add a new cli.StringFlag (name "report", Category "Output", Usage like "Path to write CSV/JSON report") to fl, then in the action flow (the function handling the send/dry-run/validate-only logic) open/create the report writer when the flag is present, ensure the writer is passed into the normalization/send pipeline (e.g., into the worker/sender functions you call) so both dry-run and full-send can append per-row results, and flush/close and write the final report on completion (skip creating the writer when validate-only is true unless explicitly requested). Use the existing identifiers fl, the action handler in batch_send.go, and the per-row send/worker functions to locate where to inject the writer.
357-364:⚠️ Potential issue | 🟠 MajorMake job submission cancellation-aware.
Once
jobs <- rowblocks, a latercancel()does not stop that handoff. With--continue-on-error=false, this can still enqueue one extra row after the first real failure and turn it into a secondarycontext canceledresult instead of a skipped row.🔧 Suggested fix
go func() { defer close(jobs) for _, row := range rows { - if workerCtx.Err() != nil { - return - } - jobs <- row + select { + case <-workerCtx.Done(): + return + case jobs <- row: + } } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send.go` around lines 357 - 364, The goroutine that sends rows into the jobs channel can block on jobs <- row and miss a later cancel(), so change the send to be cancellation-aware: inside the for loop (in the goroutine that currently uses jobs, rows, workerCtx) replace the plain jobs <- row with a select that returns if workerCtx.Done() is closed (or workerCtx.Err()!=nil) and otherwise sends the row (select { case <-workerCtx.Done(): return; case jobs <- row: }). Keep the existing defer close(jobs) and the early workerCtx.Err() check, but rely on the select to ensure a pending send doesn't ignore cancel().
🤖 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/commands/messages/batch/batch_send.go`:
- Around line 191-203: The loop over results currently calls
renderer.MessageState for every batchRowResult and can mask real send failures;
modify the loop to first check result.Error and, if non-nil, skip calling
renderer.MessageState (instead write the error to stderr with
fmt.Fprintln(os.Stderr, result.Error.Error()) or otherwise handle/collect the
error) and continue to the next result; only call renderer.MessageState when
result.Error == nil so you don't overwrite or hide the original client error.
In `@README.md`:
- Around line 241-245: Update the XLSX mapping example to remove ambiguity by
either (A) using header-based keys consistent with the rest of the docs (e.g.,
change the example command "smsgate batch send campaign.xlsx --sheet Sheet1
--map phone=A,text=B" to use header names like "--map
phone=phone_number,text=message") or (B) explicitly document Excel column-letter
selectors in the README near the example (explain that "A" and "B" refer to
Excel column letters and how they map to synthetic keys), so choose one approach
and make the command and surrounding text consistent with the rest of the
mapping documentation.
- Around line 137-142: Update the header sentence "The CLI offers four main
groups of commands:" to accurately reflect the list by changing "four" to
"three" (or alternatively add the missing fourth bullet if that group exists);
locate the sentence "The CLI offers four main groups of commands:" and either
correct it to "three main groups of commands:" or append the appropriate missing
group to the bullet list so the count matches.
- Around line 268-275: Add the omitted `id` column to the README table as an
optional field so the per-row message ID feature is documented: update the table
to include a row for `id` (mark as not required) with a brief description like
"Per-row message identifier used when `parseColumnMapping()` maps `id`; read by
`mapRow()` to set the message ID for each row" so readers know the mapping and
where it’s consumed.
---
Duplicate comments:
In `@internal/commands/messages/batch/batch_send.go`:
- Around line 315-324: The sim_number parsing in the batch row validation
(inside the block checking mapping["sim_number"]) currently accepts 0; update
the logic in this parsing branch (where strconv.ParseUint is used and
row.SimNumber is set) to treat 0 as invalid: after parsing the uint value, if
sim == 0 return an error (e.g., "invalid sim_number: must be 1-255") instead of
assigning it to sendRow.SimNumber; ensure the error is returned from the same
function so rows with sim_number=0 are rejected during validation.
- Around line 23-75: The command flags slice fl in batch_send.go lacks a
--report option and the send path never creates or writes a report artifact; add
a new cli.StringFlag (name "report", Category "Output", Usage like "Path to
write CSV/JSON report") to fl, then in the action flow (the function handling
the send/dry-run/validate-only logic) open/create the report writer when the
flag is present, ensure the writer is passed into the normalization/send
pipeline (e.g., into the worker/sender functions you call) so both dry-run and
full-send can append per-row results, and flush/close and write the final report
on completion (skip creating the writer when validate-only is true unless
explicitly requested). Use the existing identifiers fl, the action handler in
batch_send.go, and the per-row send/worker functions to locate where to inject
the writer.
- Around line 357-364: The goroutine that sends rows into the jobs channel can
block on jobs <- row and miss a later cancel(), so change the send to be
cancellation-aware: inside the for loop (in the goroutine that currently uses
jobs, rows, workerCtx) replace the plain jobs <- row with a select that returns
if workerCtx.Done() is closed (or workerCtx.Err()!=nil) and otherwise sends the
row (select { case <-workerCtx.Done(): return; case jobs <- row: }). Keep the
existing defer close(jobs) and the early workerCtx.Err() check, but rely on the
select to ensure a pending send doesn't ignore cancel().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 049c89a6-8a07-429c-8054-6aab64fd830f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
MakefileREADME.mdgo.modinternal/commands/flags/errors.gointernal/commands/flags/send.gointernal/commands/messages/batch/batch.gointernal/commands/messages/batch/batch_send.gointernal/commands/messages/batch/batch_send_test.gointernal/commands/messages/batch/batch_send_types.gointernal/commands/messages/batch/errors.gointernal/commands/messages/batch/mappings.gointernal/commands/messages/messages.gointernal/commands/messages/send.gopkg/io/tabular/csv.gopkg/io/tabular/csv_test.gopkg/io/tabular/types.gopkg/io/tabular/xlsx.gopkg/io/tabular/xlsx_test.gotests/e2e/messages_test.gouser-docs
✅ Files skipped from review due to trivial changes (13)
- user-docs
- internal/commands/flags/errors.go
- internal/commands/messages/messages.go
- internal/commands/messages/batch/errors.go
- internal/commands/messages/batch/batch_send_types.go
- pkg/io/tabular/types.go
- pkg/io/tabular/csv_test.go
- internal/commands/messages/batch/batch_send_test.go
- go.mod
- pkg/io/tabular/xlsx_test.go
- pkg/io/tabular/xlsx.go
- pkg/io/tabular/csv.go
- internal/commands/flags/send.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/commands/messages/batch/batch.go
- internal/commands/messages/batch/mappings.go
- Makefile
- tests/e2e/messages_test.go
- internal/commands/messages/send.go
capcom6
force-pushed
the
codex/plan-message-sending-implementation-from-csv
branch
from
0fc462a to
54d4836
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
README.md (1)
252-252: Minor: "XLSX sheet" is technically redundant."XLSX" expands to "Excel Spreadsheet," making "XLSX sheet" a tautology. Consider using just "Sheet name" or "Excel sheet name."
📝 Suggested fix
-| `--sheet` | XLSX sheet name (defaults to first sheet) | empty | `Sheet1` | +| `--sheet` | Sheet name (defaults to first sheet) | empty | `Sheet1` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 252, Update the option description for `--sheet` to remove the redundant "XLSX sheet" phrasing; change the text "XLSX sheet name (defaults to first sheet)" to either "Sheet name (defaults to first sheet)" or "Excel sheet name (defaults to first sheet)" so the README entry for `--sheet` is not tautological.internal/commands/messages/send.go (1)
100-100: Consider propagating the error instead of suppressing it.
NewSendFlagsis called twice: once insendBeforefor validation (result discarded), and again here with the error silently ignored. While the Before hook provides validation guarantees, suppressing errors with_is fragile if the lifecycle ever changes.♻️ Suggested improvement
Store the validated
SendFlagsin the context duringsendBeforeand retrieve it insendAction, or at minimum handle the error:- sendFlags, _ := flags.NewSendFlags(c) + sendFlags, err := flags.NewSendFlags(c) + if err != nil { + return cli.Exit(err.Error(), codes.ParamsError) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/send.go` at line 100, The call to flags.NewSendFlags in sendAction currently ignores errors (sendFlags, _ := flags.NewSendFlags(c)), which is fragile—either return/handle that error or reuse the validated SendFlags from sendBefore; modify sendBefore to call NewSendFlags once and store the resulting SendFlags in the command context (e.g., attach to c.Context or c.App.Metadata) and then in sendAction retrieve that stored SendFlags instead of calling NewSendFlags again, or if you keep the second call, check and propagate the error from NewSendFlags rather than discarding it.internal/commands/messages/batch/batch_send_test.go (1)
25-56: Extract repeated fixture setup to reduce duplication.The temp CSV creation and
findBatchSendCommand(messages.Commands())setup are repeated across tests and can be centralized into a small helper.♻️ Suggested refactor
+func setupBatchSendTest(t *testing.T) (*cli.Command, string) { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "input.csv") + require.NoError(t, os.WriteFile(path, []byte("Phone,Message\n+12025550123,Hello"), 0o600)) + + cmd, ok := findBatchSendCommand(messages.Commands()) + require.True(t, ok) + return cmd, path +} + func TestBatchSend_ValidateOnly(t *testing.T) { t.Parallel() - - dir := t.TempDir() - path := filepath.Join(dir, "input.csv") - require.NoError(t, os.WriteFile(path, []byte("Phone,Message\n+12025550123,Hello"), 0o600)) - - cmd, ok := findBatchSendCommand(messages.Commands()) - require.True(t, ok) + cmd, path := setupBatchSendTest(t) ctx := newContext(t, cmd, []string{ "--map", "phone=Phone,text=Message", @@ func TestBatchSend_InvalidMap(t *testing.T) { t.Parallel() - - dir := t.TempDir() - path := filepath.Join(dir, "input.csv") - require.NoError(t, os.WriteFile(path, []byte("Phone,Message\n+12025550123,Hello"), 0o600)) - - cmd, ok := findBatchSendCommand(messages.Commands()) - require.True(t, ok) + cmd, path := setupBatchSendTest(t)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send_test.go` around lines 25 - 56, Extract the repeated fixture setup in TestBatchSend and TestBatchSend_InvalidMap into a helper (e.g., newBatchSendFixture) that creates the temp dir, writes the CSV file, finds the command via findBatchSendCommand(messages.Commands()), and returns the created path and cmd (and ok assertion or error). Replace the duplicated lines that create dir, path, os.WriteFile, and the findBatchSendCommand(...) call in both tests with a call to this helper and use its returned values when building ctx with newContext and calling cmd.Before/cmd.Action; keep the existing flags (--map, --validate-only) and per-test differences (like different map values) in the tests themselves.
🤖 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/commands/messages/batch/mappings.go`:
- Around line 18-21: The current code trims record.Values[column] and returns
nil,nil when the value is empty, which silently hides invalid mappings (e.g.,
mapping["priority"] or mapping["sim_number"] pointing to a non-existent column).
Before calling strings.TrimSpace, check presence with the comma-ok idiom (val,
ok := record.Values[column]) and if !ok return an error indicating an invalid
mapping for the specific field (mentioning "priority" or "sim_number" as
applicable); only proceed to TrimSpace when the column key exists, and keep
returning nil,nil only for legitimately present-but-empty values if that
semantic is desired.
---
Nitpick comments:
In `@internal/commands/messages/batch/batch_send_test.go`:
- Around line 25-56: Extract the repeated fixture setup in TestBatchSend and
TestBatchSend_InvalidMap into a helper (e.g., newBatchSendFixture) that creates
the temp dir, writes the CSV file, finds the command via
findBatchSendCommand(messages.Commands()), and returns the created path and cmd
(and ok assertion or error). Replace the duplicated lines that create dir, path,
os.WriteFile, and the findBatchSendCommand(...) call in both tests with a call
to this helper and use its returned values when building ctx with newContext and
calling cmd.Before/cmd.Action; keep the existing flags (--map, --validate-only)
and per-test differences (like different map values) in the tests themselves.
In `@internal/commands/messages/send.go`:
- Line 100: The call to flags.NewSendFlags in sendAction currently ignores
errors (sendFlags, _ := flags.NewSendFlags(c)), which is fragile—either
return/handle that error or reuse the validated SendFlags from sendBefore;
modify sendBefore to call NewSendFlags once and store the resulting SendFlags in
the command context (e.g., attach to c.Context or c.App.Metadata) and then in
sendAction retrieve that stored SendFlags instead of calling NewSendFlags again,
or if you keep the second call, check and propagate the error from NewSendFlags
rather than discarding it.
In `@README.md`:
- Line 252: Update the option description for `--sheet` to remove the redundant
"XLSX sheet" phrasing; change the text "XLSX sheet name (defaults to first
sheet)" to either "Sheet name (defaults to first sheet)" or "Excel sheet name
(defaults to first sheet)" so the README entry for `--sheet` is not
tautological.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f15ef9b-0fe7-41c8-b7fd-f59b29862985
📒 Files selected for processing (14)
MakefileREADME.mdinternal/commands/flags/errors.gointernal/commands/flags/send.gointernal/commands/messages/batch/batch.gointernal/commands/messages/batch/batch_send.gointernal/commands/messages/batch/batch_send_test.gointernal/commands/messages/batch/batch_send_types.gointernal/commands/messages/batch/errors.gointernal/commands/messages/batch/mappings.gointernal/commands/messages/messages.gointernal/commands/messages/send.gotests/e2e/messages_test.gouser-docs
✅ Files skipped from review due to trivial changes (6)
- user-docs
- internal/commands/flags/errors.go
- internal/commands/messages/batch/batch.go
- internal/commands/messages/batch/batch_send_types.go
- internal/commands/messages/batch/errors.go
- internal/commands/messages/batch/batch_send.go
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/messages_test.go
- internal/commands/messages/messages.go
- Makefile
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/commands/messages/batch/batch_send_test.go (2)
84-89: Optionally silence FlagSet parse output in tests.Using
flag.ContinueOnErrorcan still write parser output to stderr; redirecting output to discard keeps test logs cleaner when negative cases are added.🧹 Small test hygiene improvement
+import "io" ... set := flag.NewFlagSet(cmd.Name, flag.ContinueOnError) + set.SetOutput(io.Discard) for _, cliFlag := range cmd.Flags { require.NoError(t, cliFlag.Apply(set)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send_test.go` around lines 84 - 89, The test creates a FlagSet with flag.ContinueOnError which can still emit parser output to stderr; after creating the FlagSet (set := flag.NewFlagSet(...)) call set.SetOutput(io.Discard) (and import io) to silence parse output, leaving the rest of the flow that applies flags (cliFlag.Apply) and calls set.Parse(args) unchanged.
39-43: Add explicit--filecoverage in CLI args.Both tests currently pass the input path positionally. Since
--fileis part of the intended public interface, add at least one test path that uses--fileto prevent regressions in flag wiring.✅ Suggested test update
ctx := newContext(t, cmd, []string{ "--map", "phone=Phone,text=Message", "--validate-only", - path, + "--file", path, })ctx := newContext(t, cmd, []string{ "--map", "text=Message", "--validate-only", - path, + "--file", path, })Also applies to: 55-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send_test.go` around lines 39 - 43, Tests in internal/commands/messages/batch/batch_send_test.go pass the input CSV path positionally; update at least one invocation that calls newContext(...) to include the explicit "--file" flag (e.g. replace the positional path arg with "--file", path) so the CLI flag wiring is exercised; modify the context creation in the test(s) that currently build args like "--map", "phone=Phone,text=Message", "--validate-only", path (and the similar call later) to instead include "--file", path while keeping other args and assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/commands/messages/batch/batch_send_test.go`:
- Around line 84-89: The test creates a FlagSet with flag.ContinueOnError which
can still emit parser output to stderr; after creating the FlagSet (set :=
flag.NewFlagSet(...)) call set.SetOutput(io.Discard) (and import io) to silence
parse output, leaving the rest of the flow that applies flags (cliFlag.Apply)
and calls set.Parse(args) unchanged.
- Around line 39-43: Tests in
internal/commands/messages/batch/batch_send_test.go pass the input CSV path
positionally; update at least one invocation that calls newContext(...) to
include the explicit "--file" flag (e.g. replace the positional path arg with
"--file", path) so the CLI flag wiring is exercised; modify the context creation
in the test(s) that currently build args like "--map",
"phone=Phone,text=Message", "--validate-only", path (and the similar call later)
to instead include "--file", path while keeping other args and assertions
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 689fd841-958e-4002-a367-29614f26184d
📒 Files selected for processing (4)
README.mdinternal/commands/messages/batch/batch_send_test.gointernal/commands/messages/batch/mappings.gointernal/commands/messages/send.go
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/commands/messages/batch/mappings.go
- internal/commands/messages/send.go
capcom6
force-pushed
the
codex/plan-message-sending-implementation-from-csv
branch
from
a483551 to
5e4b64b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/commands/flags/send.go (1)
164-164:lo.CoalesceOrEmptymay not preserve explicit zero priority.
lo.CoalesceOrEmptyreturns the first non-zero/non-empty value. Ifsrc.Priorityis explicitly0(a valid priority), it will be treated as "empty" ands.Priority(the flag default) will be used instead. This means an explicitpriority=0in the source message gets overwritten.For the single
sendcommand this is fine (src.Priority is always 0). For batch, row priority is applied after merge (line 425-427 in batch_send.go), so this is handled correctly there.Consider documenting this behavior or using a pointer type for Priority in the source to distinguish "unset" from "explicitly zero" if this becomes a concern in future use cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/flags/send.go` at line 164, The merge uses lo.CoalesceOrEmpty which treats an explicit zero as "empty" so src.Priority = 0 gets overwritten by s.Priority; update the merge to preserve explicit zero by making Priority a pointer in the source struct (e.g., change src.Priority to *int and adjust parsing) or replace lo.CoalesceOrEmpty with an explicit nil/isset check that uses src.Priority when it is explicitly set (referencing lo.CoalesceOrEmpty, src.Priority and s.Priority in send.go and the batch handling in batch_send.go) and add a short comment documenting the chosen behavior.internal/commands/messages/batch/batch_send.go (2)
376-384: Progress output accessesresult.State.Stateeven on error.When
result.Error != nil,result.Statemay be a zero-valuesmsgateway.MessageState, makingresult.State.Statepotentially empty or misleading. The error path at line 380 handles this, but accessingresult.State.Stateat line 378 before the nil check could be problematic ifStatehas other fields accessed later.♻️ Minor clarity improvement
for result := range results { - state := string(result.State.State) - if result.Error != nil { - state = fmt.Sprintf("failed: %s", result.Error.Error()) + var state string + if result.Error != nil { + state = fmt.Sprintf("failed: %s", result.Error.Error()) + } else { + state = string(result.State.State) } fmt.Fprintf(os.Stderr, "[%d] %s: %s\n", result.RowNumber, result.Identifier, state)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send.go` around lines 376 - 384, The progress printing reads result.State.State before checking result.Error, which can be misleading or unsafe when result.Error != nil and result.State is a zero-value; change the logic in the loop that consumes the results channel so you determine state after checking error: if result.Error != nil set state to a failure string like fmt.Sprintf("failed: %s", result.Error.Error()), otherwise set state = string(result.State.State), then call fmt.Fprintf and append to out; refer to the results channel loop, the batchRowResult variable result, its fields result.State and result.Error, and the fmt.Fprintf call to locate and update the code.
406-427: Row-specific fields correctly applied after merge.The priority override at lines 425-427 is correctly applied after
sendFlags.Merge(req), preserving explicit row priorities including0. However, the same pattern should be applied toDeviceIDandSimNumberto ensure row-specific values override command-line defaults.♻️ Suggested improvement for consistent row overrides
req = sendFlags.Merge(req) + if row.DeviceID != "" { + req.DeviceID = row.DeviceID + } + if row.SimNumber != nil { + req.SimNumber = row.SimNumber + } if row.Priority != nil { req.Priority = smsgateway.MessagePriority(*row.Priority) }And remove
DeviceIDandSimNumberfrom the initialreqconstruction:req := smsgateway.Message{ ID: identifier, - DeviceID: row.DeviceID, + DeviceID: "", Message: "", TextMessage: &smsgateway.TextMessage{ Text: row.Text, }, DataMessage: nil, PhoneNumbers: []string{row.Phone}, IsEncrypted: false, - SimNumber: row.SimNumber, + SimNumber: nil, WithDeliveryReport: nil, Priority: 0, TTL: nil, ValidUntil: nil, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send.go` around lines 406 - 427, The code applies row-specific Priority after calling sendFlags.Merge but sets DeviceID and SimNumber in the initial req before merging, so command-line defaults can overwrite row values; remove DeviceID and SimNumber from the initial smsgateway.Message construction and, after calling sendFlags.Merge(req), explicitly assign row overrides the same way as Priority (e.g. if row.DeviceID != "" then req.DeviceID = row.DeviceID; if row.SimNumber != nil then req.SimNumber = row.SimNumber) so row values always win over merged defaults; reference sendFlags.Merge, req.DeviceID, req.SimNumber, row.DeviceID, row.SimNumber and the existing Priority override pattern when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/commands/flags/send.go`:
- Line 164: The merge uses lo.CoalesceOrEmpty which treats an explicit zero as
"empty" so src.Priority = 0 gets overwritten by s.Priority; update the merge to
preserve explicit zero by making Priority a pointer in the source struct (e.g.,
change src.Priority to *int and adjust parsing) or replace lo.CoalesceOrEmpty
with an explicit nil/isset check that uses src.Priority when it is explicitly
set (referencing lo.CoalesceOrEmpty, src.Priority and s.Priority in send.go and
the batch handling in batch_send.go) and add a short comment documenting the
chosen behavior.
In `@internal/commands/messages/batch/batch_send.go`:
- Around line 376-384: The progress printing reads result.State.State before
checking result.Error, which can be misleading or unsafe when result.Error !=
nil and result.State is a zero-value; change the logic in the loop that consumes
the results channel so you determine state after checking error: if result.Error
!= nil set state to a failure string like fmt.Sprintf("failed: %s",
result.Error.Error()), otherwise set state = string(result.State.State), then
call fmt.Fprintf and append to out; refer to the results channel loop, the
batchRowResult variable result, its fields result.State and result.Error, and
the fmt.Fprintf call to locate and update the code.
- Around line 406-427: The code applies row-specific Priority after calling
sendFlags.Merge but sets DeviceID and SimNumber in the initial req before
merging, so command-line defaults can overwrite row values; remove DeviceID and
SimNumber from the initial smsgateway.Message construction and, after calling
sendFlags.Merge(req), explicitly assign row overrides the same way as Priority
(e.g. if row.DeviceID != "" then req.DeviceID = row.DeviceID; if row.SimNumber
!= nil then req.SimNumber = row.SimNumber) so row values always win over merged
defaults; reference sendFlags.Merge, req.DeviceID, req.SimNumber, row.DeviceID,
row.SimNumber and the existing Priority override pattern when making the
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2094ac29-ef70-4335-8408-8028ea9f2ac3
📒 Files selected for processing (14)
MakefileREADME.mdinternal/commands/flags/errors.gointernal/commands/flags/send.gointernal/commands/messages/batch/batch.gointernal/commands/messages/batch/batch_send.gointernal/commands/messages/batch/batch_send_test.gointernal/commands/messages/batch/batch_send_types.gointernal/commands/messages/batch/errors.gointernal/commands/messages/batch/mappings.gointernal/commands/messages/messages.gointernal/commands/messages/send.gotests/e2e/messages_test.gouser-docs
✅ Files skipped from review due to trivial changes (6)
- user-docs
- internal/commands/flags/errors.go
- internal/commands/messages/batch/batch.go
- internal/commands/messages/batch/batch_send_types.go
- internal/commands/messages/batch/errors.go
- internal/commands/messages/batch/batch_send_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/messages_test.go
- internal/commands/messages/messages.go
- internal/commands/messages/batch/mappings.go
- Makefile
capcom6
force-pushed
the
codex/plan-message-sending-implementation-from-csv
branch
from
e809abd to
32e7c66
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/commands/messages/batch/batch_send.go (3)
163-164:⚠️ Potential issue | 🟠 MajorAdd nil check for
client.
metadata.GetClient()can returnnilif metadata initialization failed. The client is passed torunBatchSendand used in worker goroutines, where a nil client would cause a panic.🛡️ Proposed fix
client := metadata.GetClient(c.App.Metadata) + if client == nil { + return cli.Exit("client not initialized", codes.ClientError) + } results := runBatchSend(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send.go` around lines 163 - 164, metadata.GetClient(c.App.Metadata) can return nil and the resulting client is passed into runBatchSend and used in worker goroutines; add a nil check after calling metadata.GetClient to detect a nil client, log an error (or return/exit) and avoid calling runBatchSend when client == nil so worker goroutines won't receive a nil pointer; specifically update the code around the client := metadata.GetClient(c.App.Metadata) line to handle nil before invoking runBatchSend.
122-124:⚠️ Potential issue | 🟠 MajorAdd nil check for
renderer.
metadata.GetRenderer()can returnnilif the metadata key is missing. The renderer is later used at line 196 without a guard, which would cause a panic.🛡️ Proposed fix
func batchSendAction(c *cli.Context) error { renderer := metadata.GetRenderer(c.App.Metadata) + if renderer == nil { + return cli.Exit("renderer not initialized", codes.ClientError) + } sendFlags, _ := flags.NewSendFlags(c)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send.go` around lines 122 - 124, In batchSendAction, metadata.GetRenderer(c.App.Metadata) can return nil; add a nil check for renderer after the call in batchSendAction and handle it (return a clear error from batchSendAction or set a default renderer) before any later use (e.g., where renderer is used around line 196); reference the renderer variable and ensure any subsequent calls on renderer are guarded so no nil dereference can occur.
319-329:⚠️ Potential issue | 🟠 MajorReject
sim_number=0during row validation.SIM numbers are documented as 1-based (1-255), but
strconv.ParseUint(..., 10, 8)accepts 0. A spreadsheet value of0would pass validation and be sent to the API, where it may cause unexpected behavior.🛡️ Proposed fix
if column, ok := mapping["sim_number"]; ok { raw := strings.TrimSpace(record.Values[column]) if raw != "" { sim, err := strconv.ParseUint(raw, 10, 8) if err != nil { return sendRow{}, fmt.Errorf("invalid sim_number: %w", err) } + if sim == 0 { + return sendRow{}, fmt.Errorf("%w: sim_number must be between 1 and 255", ErrValidationFailed) + } sim8 := uint8(sim) row.SimNumber = &sim8 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send.go` around lines 319 - 329, The code accepts "0" as a valid sim_number because strconv.ParseUint(..., 10, 8) will parse 0; modify the validation in the block handling mapping["sim_number"] (the ParseUint call and subsequent sim8 assignment to row.SimNumber) to explicitly reject 0 and out-of-range values: after parsing (the variable named sim), if sim == 0 || sim > 255 return a descriptive error like "invalid sim_number: must be 1-255", otherwise cast to uint8 (sim8 := uint8(sim)) and set row.SimNumber = &sim8.
🤖 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/commands/messages/batch/batch_send.go`:
- Around line 354-359: The code uses sync.WaitGroup.Go(), which requires Go
1.25+, but the project README claims Go 1.23+; either update docs to require Go
1.25+ or switch to the traditional pattern for compatibility: replace the
wg.Go(...) loop with wg.Add(1) and spawn a goroutine that defers wg.Done() and
calls sendWorker(workerCtx, client, jobs, results, sendFlags, continueOnError,
cancel) (use the existing symbols wg, sendWorker, workerCtx, client, jobs,
results, sendFlags, continueOnError, cancel) so the code compiles under Go
1.23/1.24.
---
Duplicate comments:
In `@internal/commands/messages/batch/batch_send.go`:
- Around line 163-164: metadata.GetClient(c.App.Metadata) can return nil and the
resulting client is passed into runBatchSend and used in worker goroutines; add
a nil check after calling metadata.GetClient to detect a nil client, log an
error (or return/exit) and avoid calling runBatchSend when client == nil so
worker goroutines won't receive a nil pointer; specifically update the code
around the client := metadata.GetClient(c.App.Metadata) line to handle nil
before invoking runBatchSend.
- Around line 122-124: In batchSendAction, metadata.GetRenderer(c.App.Metadata)
can return nil; add a nil check for renderer after the call in batchSendAction
and handle it (return a clear error from batchSendAction or set a default
renderer) before any later use (e.g., where renderer is used around line 196);
reference the renderer variable and ensure any subsequent calls on renderer are
guarded so no nil dereference can occur.
- Around line 319-329: The code accepts "0" as a valid sim_number because
strconv.ParseUint(..., 10, 8) will parse 0; modify the validation in the block
handling mapping["sim_number"] (the ParseUint call and subsequent sim8
assignment to row.SimNumber) to explicitly reject 0 and out-of-range values:
after parsing (the variable named sim), if sim == 0 || sim > 255 return a
descriptive error like "invalid sim_number: must be 1-255", otherwise cast to
uint8 (sim8 := uint8(sim)) and set row.SimNumber = &sim8.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44398887-1b52-44d1-8a24-c5b57a7286f8
📒 Files selected for processing (14)
MakefileREADME.mdinternal/commands/flags/errors.gointernal/commands/flags/send.gointernal/commands/messages/batch/batch.gointernal/commands/messages/batch/batch_send.gointernal/commands/messages/batch/batch_send_test.gointernal/commands/messages/batch/batch_send_types.gointernal/commands/messages/batch/errors.gointernal/commands/messages/batch/mappings.gointernal/commands/messages/messages.gointernal/commands/messages/send.gotests/e2e/messages_test.gouser-docs
✅ Files skipped from review due to trivial changes (8)
- user-docs
- internal/commands/messages/batch/batch_send_types.go
- internal/commands/flags/errors.go
- internal/commands/messages/batch/batch.go
- tests/e2e/messages_test.go
- internal/commands/messages/batch/errors.go
- internal/commands/messages/batch/mappings.go
- internal/commands/flags/send.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/commands/messages/messages.go
- Makefile
- internal/commands/messages/batch/batch_send_test.go
- internal/commands/messages/send.go
capcom6
force-pushed
the
codex/plan-message-sending-implementation-from-csv
branch
from
c0066f9 to
9cc7c28
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/commands/messages/batch/batch_send_test.go (1)
33-64: Add one regression here for the shared send-flag parser.
batch sendnow depends onflags.NewSendFlags(...)in itsBefore, but this suite only exercises--mapand--validate-only. A case like--sim-number 0or fractional--ttlwould keep a parser regression from slipping through with these tests still green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/batch_send_test.go` around lines 33 - 64, The test suite misses a regression for the shared send-flag parser used in batch send (flags.NewSendFlags called in the command's Before), so add a test that invokes the command's Before with send-flag inputs (e.g., invalid values like "--sim-number 0" or fractional "--ttl 1.5" and a valid case) to ensure the parser validates them; locate the BatchSend tests (e.g., TestBatchSend_ValidateOnly and TestBatchSend_InvalidMap) and add a new test that builds a context with those flags, calls cmd.Before(ctx), and asserts error for invalid flags and no error for valid flags to prevent parser regressions.
🤖 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/commands/flags/send.go`:
- Around line 100-109: The validation currently only rejects values >
maxSIMNumber so an explicit --sim-number 0 slips through and becomes nil via
lo.EmptyableToPtr, causing silent fallback; update the validation in the block
that reads simNumber to explicitly reject simNumber == 0 by returning
ErrValidationFailed with a message stating sim number must be between 1 and
maxSIMNumber (include the provided simNumber), before assigning fl.SimNumber, so
zero is treated as invalid rather than converted to nil.
In `@internal/commands/messages/send.go`:
- Around line 65-67: Parse SendFlags exactly once in the command's Before hook
by calling flags.NewSendFlags(c) there, store the returned *SendFlags value in
the CLI context (e.g., attach it to c.Context via context.WithValue or command
metadata) and then retrieve and reuse that same parsed flags instance inside
sendAction (and any other hooks) instead of calling NewSendFlags again; update
both call sites (the current NewSendFlags invocation in Before and the separate
invocation in sendAction / lines 100-103) to use the stored value so the
time-sensitive valid-until check is performed only once.
---
Nitpick comments:
In `@internal/commands/messages/batch/batch_send_test.go`:
- Around line 33-64: The test suite misses a regression for the shared send-flag
parser used in batch send (flags.NewSendFlags called in the command's Before),
so add a test that invokes the command's Before with send-flag inputs (e.g.,
invalid values like "--sim-number 0" or fractional "--ttl 1.5" and a valid case)
to ensure the parser validates them; locate the BatchSend tests (e.g.,
TestBatchSend_ValidateOnly and TestBatchSend_InvalidMap) and add a new test that
builds a context with those flags, calls cmd.Before(ctx), and asserts error for
invalid flags and no error for valid flags to prevent parser regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc30152b-72a3-4b8e-9df1-13d79636aaed
📒 Files selected for processing (15)
DockerfileMakefileREADME.mdinternal/commands/flags/errors.gointernal/commands/flags/send.gointernal/commands/messages/batch/batch.gointernal/commands/messages/batch/batch_send.gointernal/commands/messages/batch/batch_send_test.gointernal/commands/messages/batch/batch_send_types.gointernal/commands/messages/batch/errors.gointernal/commands/messages/batch/mappings.gointernal/commands/messages/messages.gointernal/commands/messages/send.gotests/e2e/messages_test.gouser-docs
✅ Files skipped from review due to trivial changes (6)
- user-docs
- Dockerfile
- internal/commands/flags/errors.go
- internal/commands/messages/batch/batch.go
- internal/commands/messages/batch/batch_send_types.go
- internal/commands/messages/batch/errors.go
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/messages_test.go
- internal/commands/messages/messages.go
- README.md
- Makefile
- internal/commands/messages/batch/mappings.go
- internal/commands/messages/batch/batch_send.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/commands/messages/batch/send_test.go (1)
81-91: Test context lacks App metadata, limiting coverage.The constructed
cli.Contexthas an emptycli.App{}with nilMetadata. This works for--validate-only(which returns before usingrenderer/client), but tests for the actual send path would panic. Consider adding a mock renderer/client toApp.Metadataif expanding test coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/send_test.go` around lines 81 - 91, newContext currently returns cli.NewContext(&cli.App{}, ...) with an empty cli.App so App.Metadata is nil; that will panic when tests exercise the send path that expects metadata like a renderer or client. Modify newContext to construct a cli.App with a non-nil Metadata map and populate it with lightweight mocks/stubs for the dependencies the send command reads (e.g. the renderer and message client entries used by the send code), so cli.Context returned by newContext contains the same Metadata keys/values the real command expects (reference newContext, cli.App, App.Metadata).
🤖 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/commands/messages/batch/mappings/mappings.go`:
- Around line 127-128: The mapRow function is double-wrapping the error from
parsePriority (parsePriority already returns errors wrapped with "invalid
priority: %w"), so in mapRow (where you currently do: if priority, err :=
parsePriority(...); err != nil { return SendRow{}, fmt.Errorf("invalid priority:
%w", err) }) stop re-wrapping and simply propagate the original error (return
SendRow{}, err) or otherwise return the wrapped error unchanged; adjust mapRow
to reference parsePriority and SendRow accordingly so you don't produce messages
like "invalid priority: invalid priority: ...".
---
Nitpick comments:
In `@internal/commands/messages/batch/send_test.go`:
- Around line 81-91: newContext currently returns cli.NewContext(&cli.App{},
...) with an empty cli.App so App.Metadata is nil; that will panic when tests
exercise the send path that expects metadata like a renderer or client. Modify
newContext to construct a cli.App with a non-nil Metadata map and populate it
with lightweight mocks/stubs for the dependencies the send command reads (e.g.
the renderer and message client entries used by the send code), so cli.Context
returned by newContext contains the same Metadata keys/values the real command
expects (reference newContext, cli.App, App.Metadata).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f89f0463-6e2f-442b-b84f-8ee12e41923f
📒 Files selected for processing (8)
internal/commands/messages/batch/errors.gointernal/commands/messages/batch/mappings/errors.gointernal/commands/messages/batch/mappings/mappings.gointernal/commands/messages/batch/mappings/types.gointernal/commands/messages/batch/send.gointernal/commands/messages/batch/send_test.gointernal/commands/messages/batch/types.gointernal/commands/messages/send.go
✅ Files skipped from review due to trivial changes (2)
- internal/commands/messages/batch/types.go
- internal/commands/messages/send.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/commands/messages/batch/mappings/mappings.go (1)
48-48: Reject duplicate mapping keys instead of silently overriding.Line 48 overwrites previous values for the same field (for example, repeated
phone=). Failing fast here makes operator mistakes explicit.🔧 Suggested fix
if _, ok := allowed[key]; !ok { return nil, fmt.Errorf("%w: unsupported mapping field %q", ErrMappingParseFailed, key) } + if _, exists := result[key]; exists { + return nil, fmt.Errorf("%w: duplicate mapping field %q", ErrMappingParseFailed, key) + } result[key] = column }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/mappings/mappings.go` at line 48, The code currently overwrites existing entries when assigning result[key] = column; update the logic to detect duplicates by checking if key already exists in the result map (e.g., if _, exists := result[key]; exists { ... }) and fail fast instead of silently overriding: return an error (or propagate one) that includes the duplicate key and the conflicting column mapping. Modify the enclosing function's signature to return an error if it doesn't already, and ensure callers handle the new error path; keep references to the variables result, key, and column when implementing the check and error message.
🤖 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/commands/messages/batch/mappings/mappings.go`:
- Around line 115-124: The sim_number mapping currently reads
record.Values[column] without validating that column is a valid index, so a
missing column is treated as empty and silently ignored; update the code around
the mapping["sim_number"] block to check that the resolved column index is
within bounds of record.Values (e.g., column >= 0 and column <
len(record.Values)) and return a clear error (from the function that builds
SendRow) when the mapped column is missing or out of range, rather than treating
it as empty; ensure you still parse and set row.SimNumber as uint8 when present.
---
Nitpick comments:
In `@internal/commands/messages/batch/mappings/mappings.go`:
- Line 48: The code currently overwrites existing entries when assigning
result[key] = column; update the logic to detect duplicates by checking if key
already exists in the result map (e.g., if _, exists := result[key]; exists {
... }) and fail fast instead of silently overriding: return an error (or
propagate one) that includes the duplicate key and the conflicting column
mapping. Modify the enclosing function's signature to return an error if it
doesn't already, and ensure callers handle the new error path; keep references
to the variables result, key, and column when implementing the check and error
message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d29b19e-116a-4ba3-baaf-368aff45a4c4
📒 Files selected for processing (1)
internal/commands/messages/batch/mappings/mappings.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/commands/messages/batch/mappings/mappings.go (1)
118-128:⚠️ Potential issue | 🟠 Major
sim_numbermapping doesn't validate column existence (inconsistent withparsePriority).When
mapping["sim_number"]is set but the column doesn't exist inrecord.Values, the code silently treats it as no SIM selection rather than reporting a mapping error. This is inconsistent withparsePriority(lines 83-86) which properly validates column existence.This can hide typos in
--mapvalues (e.g.,sim_number=SIMwhen the actual column isSimNumber).🔧 Suggested fix for consistency with parsePriority
if column, ok := mapping["sim_number"]; ok { - raw := strings.TrimSpace(record.Values[column]) + value, exists := record.Values[column] + if !exists { + return SendRow{}, fmt.Errorf("%w: invalid sim_number mapping: column %q not found", ErrMappingParseFailed, column) + } + raw := strings.TrimSpace(value) if raw != "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/messages/batch/mappings/mappings.go` around lines 118 - 128, The sim_number mapping handling must validate that the column index from mapping["sim_number"] actually exists in record.Values (like parsePriority does) instead of silently skipping when out of range; update the block that reads mapping["sim_number"] so it checks the column index is within bounds of record.Values and returns a mapping error (fmt.Errorf) if not, then proceed to trim, parse with strconv.ParseUint, convert to uint8 and assign to SendRow.SimNumber as before; follow the same existence/validation pattern used by parsePriority to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/commands/messages/batch/mappings/mappings.go`:
- Around line 118-128: The sim_number mapping handling must validate that the
column index from mapping["sim_number"] actually exists in record.Values (like
parsePriority does) instead of silently skipping when out of range; update the
block that reads mapping["sim_number"] so it checks the column index is within
bounds of record.Values and returns a mapping error (fmt.Errorf) if not, then
proceed to trim, parse with strconv.ParseUint, convert to uint8 and assign to
SendRow.SimNumber as before; follow the same existence/validation pattern used
by parsePriority to keep behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6ae4927-7a6a-4de2-b039-3220ed887504
📒 Files selected for processing (1)
internal/commands/messages/batch/mappings/mappings.go
capcom6
force-pushed
the
codex/plan-message-sending-implementation-from-csv
branch
from
ba2be45 to
d1fb819
Compare
capcom6
changed the title
batch send command with CSV/XLSX tabular readers and reportingbatch send command
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.
Motivation
sendbehavior remains stable.Description
batchcommand with asendsubcommand exposing flags like--file,--sheet,--map,--delimiter,--header,--dry-run,--validate-only,--concurrency,--continue-on-error, and--report.internal/io/tabular) with CSV (NewCSVReader) and XLSX (NewXLSXReader) implementations and a normalizedRecordtype.internal/commands/messages/batch_send.goand types) that parses column mappings, maps/validates rows intoBatchSendRow, runs a bounded-concurrency worker pool to call the client, and emits per-row results and JSON/CSV reports.newTextMessageRequesthelper (internal/commands/messages/send_request.go) and refactor the single-messagesendpath to use it.docs_batch_sending_plan.mddescribing approach and options.Testing
internal/io/tabular(csv_test.go,xlsx_test.go) and for the command wiring ininternal/commands/messages/batch_send_test.go; tests exercise header/no-header parsing and basic command--validate-onlybehavior.go test ./...to validate the new packages and command integration, and all tests completed successfully.Codex Task
Summary by CodeRabbit
New Features
Documentation
Chores