Refactoring cleanup: improve code quality with roborev analyze #32
+15,260
−10,820
Conversation
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
Propagate a context.Context that listens for SIGINT/SIGTERM from main() through Cobra's ExecuteContext, enabling commands like sync and export to clean up resources when interrupted. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… DatabasePath - Extract NewDefaultConfig() to separate default creation from Load logic - Fix expandPath to only expand "~" or "~/" prefixes, not "~filename" - Rename DatabasePath() to DatabaseDSN() to clarify it returns a DSN, not just a path Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Extract four helpers from Execute and ExecuteBatch: - deleteOne: centralizes single-message deletion, error classification (404-as-success, scope errors as fatal), and DB marking - saveCheckpoint: consolidates repeated manifest checkpoint logic - prepareExecution: shared manifest loading, status validation, and InProgress transition - finalizeExecution: shared completion status, manifest save, and move Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ertion helpers - Consolidate Execute and ExecuteBatch scenario tests into table-driven TestExecutor_Execute_Scenarios and TestExecutor_ExecuteBatch_Scenarios - Add AssertIsScopeError helper to centralize scope error string matching - Add GetBatchDeleteCall helper for safe mock slice access with bounds checking - Use msgIDs() consistently instead of manual slice literals Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…strings.Builder - Add persistedStatuses slice and dirForStatus() as single source of truth for status-to-directory mapping, replacing duplicated switch/list logic - Simplify NewManager, SaveManifest, GetManifest to use shared helpers - Replace string concatenation with strings.Builder in FormatSummary - Replace imperative sanitizeForFilename loop with strings.Map - Log warnings for corrupt manifest files instead of silently skipping Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
….IsSortedFunc Replace manual field-by-field AssertManifestEqual with go-cmp structural comparison (auto-covers new fields). Consolidate three separate state transition tests into a single table-driven test. Simplify sort verification using slices.IsSortedFunc. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…integrate SanitizeFilename - Extract addAttachmentToZip and resolveUniqueFilename helpers to reduce cyclomatic complexity and enable safe defer for file closing - Replace AttachmentResult (formatted string) with ExportStats struct, moving presentation logic to FormatExportResult - Integrate SanitizeFilename into resolveUniqueFilename to strip invalid characters from zip entry names - Update caller in tui/actions.go and tests accordingly Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…contents, use shared assertions - Add test cases for successful single and multiple file exports - Add mixed valid/invalid attachment test case - Verify exported files exist in the zip (side-effect validation) - Replace local assertContainsSubstrings with testutil.AssertContainsAll - Use setup func pattern to create real content-addressed attachment files Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move sync-related types (SyncProgress, SyncSummary, SyncProgressWithDate, NullProgress) to sync_types.go to separate progress reporting from API contract definition. Split the monolithic API interface into composable sub-interfaces (AccountReader, MessageReader, MessageDeleter) following the Interface Segregation Principle, enabling more precise dependency injection. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…tract history mapper Replace manual base64 padding (padBase64) with base64.RawURLEncoding from the standard library. Extract anonymous JSON response structs to named package-level types for reusability and readability. Extract ListHistory mapping logic into dedicated mapHistoryEntries/mapMessageChanges/mapLabelChanges helper functions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace hardcoded 403 with http.StatusForbidden and extract duplicated slice-to-map conversion logic into toReasonMaps helper. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…constants - Replace sync/atomic with plain int for rateLimitCallCount (already mutex-protected) - Extract checkErrors helper to deduplicate transient/permanent error logic in TrashMessage and DeleteMessage - Define OpTrash, OpDelete, OpBatchDelete constants replacing magic strings Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… table-drive GetCallCount - Reset test now dirties all fields (TrashCalls, DeleteCalls, BatchDeleteCalls, CallSequence, hooks) and verifies each is cleared - Hooks test uses a single table-driven approach covering both allow and block paths for all three hook types - GetCallCount test uses table-driven assertions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…es helper Document that GetMessagesRawBatch intentionally leaves nil entries for failed fetches (matching the real Client behavior). Add thread-safe SetupMessages helper for configuring pre-built RawMessage values. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ve ensureClock - Refactor Acquire into reserve/wait pattern: separate token reservation logic (under lock) from the wait loop for clarity and testability - Replace operationCosts map with switch statement in Cost() to eliminate redundant data source and map lookup overhead - Remove ensureClock defensive initialization since NewRateLimiter always sets the clock; remove corresponding nil-clock test - Extract magic numbers into named constants (defaultQPS, throttleRecoveryFactor, minWait) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…tor, consolidated helpers - Replace polling loops (runtime.Gosched/time.Sleep) in waitForTimers and acquireAsync with channel-based timerNotify for deterministic synchronization - Extract newRateLimiter(clk, qps) in production code so tests reuse the same initialization logic instead of duplicating defaults - Replace standalone setTokens/getRefillRate/getThrottledUntil helpers with fixture.drain() and fixture.state() snapshot method Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add getIDArg and getDateArg helpers to consolidate duplicated argument parsing across getMessage, getAttachment, listMessages, and aggregate handlers - Extract readAttachmentFile method to separate file I/O concerns from the getAttachment request handler - Rename intArg to limitArg to clarify its clamping behavior Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Deduplicate limit/offset/after/before argument definitions into shared helper functions (withLimit, withOffset, withAfter, withBefore). Define tool name constants to reduce magic strings in tool registration. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…hment errors, add helper - Extract statsResponse and attachmentResponse types to replace inline anonymous structs in runTool calls - Add newTestHandlers helper to reduce repetitive mock+handler initialization - Consolidate TestGetAttachment error cases into table-driven tests with custom engine config Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…or tag stripping - Extract processParts() to deduplicate attachment/inline iteration in Parse() - Move dateFormats slice to package level to avoid re-allocation per call - Use strings.Fields/Join for O(N) whitespace normalization in parseDate and StripHTML - Replace manual tag-stripping state machine with compiled regex for correctness Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ble, remove redundant helpers - Add real assertions to group address tests instead of just logging diagnostics - Consolidate date parsing tests into a single table-driven test with expected values - Replace verbose manual assertions in TestParse_MinimalMessage with assertAddress helper - Remove redundant assertStringSliceEqual wrapper and unused date assertion helpers Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…t callback handler - Use local ServeMux instead of http.DefaultServeMux to prevent panics when Authorize is called multiple times in the same process - Delegate NewManager to NewManagerWithScopes to remove duplicated logic - Extract callback handler into newCallbackHandler method for clarity - Define constants for redirect port and callback path Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ta test Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… into table-driven dispatch - Add aggViewDef struct and getViewDef() to define per-view query variations (key expression, JOIN clause, null guard, search key columns) - Add runAggregation() generic method that assembles and executes aggregate SQL - Replace 7 near-identical AggregatBy* methods with thin wrappers calling aggregateByView() - Replace 180-line SubAggregate switch with single call to runAggregation() - Extract timeExpr() helper to eliminate 4 duplicated time granularity switches - Extract inferTimeGranularity() to deduplicate period-length-based granularity inference Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…h TestDataBuilder - Replace duplicated assertMessageIDs/assertStringIDs with generic assertSetEqual[T] - Rewrite EmptyStringFallback and MatchEmptySenderName tests to use typed TestDataBuilder instead of raw SQL strings, improving readability Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace AggregateBySender, AggregateBySenderName, AggregateByRecipient, AggregateByRecipientName, AggregateByDomain, AggregateByLabel, and AggregateByTime with a single Aggregate(ctx, ViewType, opts) method on the Engine interface. This reduces the interface surface from 15+ methods and eliminates repetitive switch statements at every call site. - SQLiteEngine.Aggregate dispatches to unexported per-view methods - DuckDBEngine.Aggregate delegates to existing aggregateByView - Remove GetAggregateFunc and AggregateFunc type from engine.go - Update all callers: TUI, MCP handlers, CLI commands, mock, tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…et, extract sub-structs Replace 6 mutually-exclusive MatchEmpty* boolean fields with a single EmptyValueTarget *ViewType pointer, enforcing the "only one at a time" constraint at the type level. Extract Pagination, MessageSorting, and TimeRange structs from MessageFilter to improve readability and clarify intent. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ith table-driven dispatch Extract aggDimension struct that captures the variable parts (key expression, joins, where clause) per ViewType. Both Aggregate and SubAggregate now share a single executeAggregate path via buildAggregateSQL. ListMessages reuses buildFilterJoinsAndConditions instead of duplicating filter logic. Removes buildWhereClause (replaced by optsToFilterConditions). Net -586 lines. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…SubAggregate into table-driven test - Replace manual map/index checks in TestAggregateByTime, TestAggregateWithDateFilter, and TestSortingOptions with assertAggRows helper for consistent assertions - Consolidate 6 individual SubAggregate tests into a single table-driven TestSubAggregates - Add subtests to TestSortingOptions for clarity Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ts, standardize BCC test setup - Consolidate ~15 separate TestListMessages/TestMatchEmpty filter functions into two table-driven tests (TestListMessages_Filters and TestListMessages_MatchEmptyFilters) - Standardize TestRecipientNameFilter_IncludesBCC to use newTestEnv helper instead of manually constructing dbtest.NewTestDB - Extract viewTypePtr helper to reduce verbose EmptyValueTarget construction - Remove unused context import Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Assert exact increment (+1) of searchRequestID and loadRequestID instead of checking for specific values (was assuming 0 -> 1) - Add command assertions (assertCmd) for drill-down tests to verify that commands are returned for loading messages - Add command assertions for activateInlineSearch calls to verify the textinput.Blink command is returned This makes tests more robust by capturing initial values before operations and verifying exact increments, following the pattern already established in TestInlineSearchTabToggle. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The TestModelBuilder used `b.viewType != 0` to detect explicit viewType settings, but ViewSenders is iota 0, causing WithViewType(ViewSenders) to be silently ignored. Add viewTypeSet flag to track explicit calls, matching the pattern already used for selectedAggregates. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Anchor prereleaseNumericPattern to avoid partial matches (e.g., "rc10a") - Process each dot-separated prerelease identifier independently so versions like "1.0.0-rc10.1" normalize correctly - Skip normalization for identifiers with leading zeros to avoid creating invalid semver numeric identifiers - Fix backup restoration test to exercise the intended code path by making the source file unreadable (fails copy) rather than making the directory read-only (fails rename before copy) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace tests that only verified unknown commands return errors with tests that actually verify context propagation: - TestExecuteContext_PropagatesContext: Verifies context values are passed through to command handlers by checking a custom context key - TestExecute_UsesBackgroundContextInHandler: Verifies handlers receive a proper background context with no deadline Both tests now save/restore the global rootCmd to prevent state leakage across tests, addressing a source of potential test interference. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The ExportAttachments function was setting Err for any non-empty stats.Errors, which caused handleExportResult to show only "Export failed: export failed" and discard the detailed Result message. This hid the zip path and error details for partial-success exports where some files exported successfully. Changed to only set Err for true failures (write errors or zero exported files). Partial success now shows the detailed Result which includes both success info and error list via FormatExportResult. Added tests for partial success and full success scenarios. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The test cases "URL-safe characters padded" and "URL-safe dash with padding" were incorrectly labeled - the inputs (Pz8_, Pj4-) have no padding chars (=) so they exercise RawURLEncoding, not URLEncoding. Renamed to accurately describe them as unpadded, and added three new test cases that exercise the URLEncoding path with inputs containing both padding (=) and URL-safe characters (- or _): - "-A==" (0xf8): URL-safe dash with double padding - "_w==" (0xff): URL-safe underscore with double padding - "A-A=" (0x03 0xe0): URL-safe dash with single padding Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Restore the bounds check for the idx parameter to prevent panics when idx is out of range. The length check alone is insufficient since a caller could pass an invalid idx even when the slice length matches. Using Fatalf ensures the test fails cleanly instead of panicking. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace non-blocking default cases with 10ms timeouts when asserting that codeChan/errChan remain empty. This catches late/async sends that a non-blocking check would miss, making the tests more robust against regressions if the handler implementation changes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use t.Fatal for nil error checks to fail fast and remove the redundant nil guard on the strings.Contains check. This aligns with the SQLite test pattern and simplifies the error assertion logic. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace raw SQL INSERT with hardcoded IDs with AddSource, AddConversation, and AddParticipant helpers. This decouples the test from seed ordering and follows the established pattern in other tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The test previously only checked that AfterDate was not too far in the past (now-2d), which would pass even if the parser incorrectly returned time.Now() or a future date. Now assert AfterDate falls within a tight window around the expected value (now-24h ± 12h) and is not in the future. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Guard against typed nil *sqlite3.Error in isSQLiteError to prevent potential panic when errors.As succeeds but the underlying pointer is nil. Also add comprehensive unit tests for the function covering both value and pointer forms of sqlite3.Error, typed nil pointers, and non-SQLite errors. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The test was a no-op: it set up test fixtures but never called parseDBTime or asserted any error behavior. Now it directly tests parseDBTime via an exported test helper and verifies the error includes the bad timestamp value. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The direct equality check would fail if InspectMessage ever wraps the error with %w. Using errors.Is handles wrapped errors correctly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace zero-value mutation with increment/toggle approach that guarantees actual value changes even when original values are zero. Add documentation for unexported field skip behavior in tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ests
Using &testing.T{} as a mock is unsafe because it relies on unexported
internals and can behave incorrectly if Fatal/FailNow is called (they
invoke runtime.Goexit). Replace with a minimal errRecorder stub that
properly implements testing.TB and records Errorf calls.
Also changes AssertStringSet to accept testing.TB instead of *testing.T
to support the stub, and adds a test case for nil vs empty slice.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Cover the new prerelease normalization behavior including: - Multi-part identifiers (rc10.1 → rc.10.1) - Alphanumeric suffixes staying unchanged (rc10a) - Leading zeros skipped to avoid invalid semver (rc01, beta007) - Mixed identifiers (alpha10.beta2 → alpha.10.beta.2) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add explicit comments warning against t.Parallel() usage in tests that modify the package-level rootCmd variable. This prevents potential test flakiness from data races if future developers add parallel test execution. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace inline 10ms timeouts in TestNewCallbackHandler with a reusable generic helper that uses 100ms to reduce flakiness on slow CI runners while still detecting late asynchronous sends. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Capture and use returned IDs from AddSource and AddConversation instead of relying on implicit defaults (SourceID=1, ConversationID=1). This removes coupling to helper defaults and SQLite auto-increment assumptions, making the test more robust against future changes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix pointer slice elements pointing to structs by using mutateStruct helper and falling back to nil assignment when struct mutation fails - Change warnings to t.Fatalf when mutation is impossible (e.g., structs with only unexported fields) to make coverage gaps visible - Extend mutateValue to handle complex, map, slice, array, interface, chan, and func kinds with appropriate mutation strategies - Add mutateStruct helper that attempts to mutate at least one settable field and returns success/failure status - Return bool from mutateValue to indicate mutation success Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The errRecorder test stub embedded testing.TB but left it nil, meaning any call to unimplemented TB methods would panic. Now errRecorder wraps a real testing.TB via newErrRecorder(t), so unoverridden methods delegate safely. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Map mutation now guarantees semantic changes for all states: nil maps become non-nil, empty non-nil maps become nil, and non-empty maps have a key deleted (instead of just creating a new empty map which was a no-op) - mutateSliceElement now checks mutateValue return value and falls back to setting the pointer to nil when mutation fails (e.g., pointer to nil interface or nil func) - Add unit tests covering map edge cases and pointer-to-nil-interface/func scenarios to ensure mutation actually changes the encoded output Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Rename TestMutateValuePointerToNilInterface to TestMutateValueNilInterface since it tests direct interface fields, not pointers - Rename TestMutateValuePointerToNilFunc to TestMutateValueNilFunc for the same reason - Add TestMutateSliceElementPointerToNilInterface and TestMutateSliceElementPointerToNilFunc to exercise the pointer fallback branch in mutateSliceElement (lines 162-166) that sets pointers to nil when mutateValue fails on the underlying value - Add TestMutateValueMapEncodingChange to verify map mutations produce different encoded output, catching cases where nil/empty toggling might be a no-op for encoders with omitempty - Add clarifying comment to TestMutateValueMapEdgeCases about the nil/empty toggle limitation - Remove redundant TestMutateSliceElementPointerFallback (superseded by more specific tests) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The reflection-based cloning in EncodedSamples was over-engineered: - Required 7 "hardening" commits to handle edge cases - Spawned 400 lines of mutation test helpers to test the reflection code - Solved a problem that doesn't exist (compiler catches missing fields) Reverted to simple explicit field copying with maintainer comments to prevent future over-engineering. Kept the useful long Asian encoding samples that were added. Before: 576 lines (encoding.go + encoding_test.go) After: 225 lines (2.5x reduction) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Tests that exercise ExportAttachments were leaving Test_1.zip in the source directory because the export function writes to the current working directory. Added t.Cleanup() calls to remove the artifact. TODO comment added noting that ExportAttachments should write to a configurable output directory. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
repair_encoding.go had local copies of ensureValidUTF8, sanitizeUTF8, detectAndDecode, and getEncodingByName that duplicated the same functions in internal/textutil/encoding.go. - Replace local ensureValidUTF8() with textutil.EnsureUTF8() - Delete 122 lines of duplicate function implementations - Delete repair_encoding_test.go (215 lines) - covered by textutil tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The comment claimed the compiler would catch missing fields via "unkeyed struct literal" but the code uses keyed literals (compiler won't warn). Updated to accurately describe the actual safety mechanism: TestEncodedSamplesNonEmpty catches missing []byte fields. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
wesm
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains targeted refactoring guided by:
roborev analyze refactor --per-file --agent gemini '*/*/*.go'🤖 Generated with Claude Code