Conversation
dhth
commented
Nov 18, 2025
- add diff config type
- add tests
- compute diff
- add diffs to html output
- minor fixes
- don't include results where baseRef == headRef
- render diff without escaping symbols
- add --include-diffs flag
- fix diff syntax highlighting
- add highlighting to stdout output
📝 WalkthroughWalkthroughAdds per-comparison diff support: new DiffConfig parsing/validation and DiffResult type wired into ModuleResult. Extends Comparison payload and validation to carry diff config. CLI gains --include-diffs flag and GetComparisonResult/buildComparisonResult signatures now accept an include-diffs/diffCfg parameter. Implements generateDiff to run an external diff command and attach outputs to results. View layers updated (HTML template, HTML renderer, stdout renderer) to present diffs with optional syntax highlighting. New tests cover config parsing and HTML rendering with diffs. Go module deps updated for highlighting. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/services/compare_modules_test.go (1)
37-37: Tests correctly updated for new signatures; consider adding diff-enabled coverageThe test updates match the new APIs (
GetComparisonResultandbuildComparisonResult) and correctly assert the new error return frombuildComparisonResult. All existing behaviors are still covered forincludeDiffs == false. As a follow-up, it could be useful to add at least one test that exercisesincludeDiffs == truewith a simpleDiffConfigto validate the diff path and error propagation.Also applies to: 68-68, 96-96, 133-136, 145-148
internal/view/types.go (1)
3-6: HTMLDiff wiring is clear; ensure diff HTML is from a trusted sourceThe new
HTMLDiffstruct andHTMLData.Diffsfield cleanly mirror the domain diff data. SinceOutputistemplate.HTML, auto-escaping is bypassed, so just ensure the diff HTML comes from a trusted/highly controlled generator (as it appears to) rather than arbitrary user input.Also applies to: 17-17, 26-33
internal/view/stdout.go (1)
4-4: Stdout diff rendering and highlighting are well-structured, with graceful fallbackThe refactor to build the table plus per-module diff blocks into a
strings.Builderand then write once keeps the rendering logic straightforward, and theDiffResultblocks (with labels/refs and optional highlighting) are wired correctly. ThehighlightDiffhelper’s use of chroma is robust: if highlighting fails, it returns the plain diff, so rendering cannot break solely due to theming. If you expect very large diffs, you might later consider streaming directly towriterinstead of buffering the entire output, but for typical Terraform module diffs this approach is perfectly reasonable.Please double-check against the
github.com/alecthomas/chroma/v2/quickdocumentation that the"diff"lexer and"gruvbox"style are available in your chosen version, and thatterminal16moutput looks correct in your target terminals.Also applies to: 8-8, 10-10, 16-16, 74-79, 80-101, 103-105, 111-119
internal/services/compare_modules.go (2)
17-17: DiffConfig gating and error propagation are correct; confirm desired failure behaviorThe wiring of
includeDiffs→diffCfg→buildComparisonResultlooks correct: diffs are only attempted when both the flag is true and aDiffCfgis present, and only for modules that areStatusOutOfSyncwith both base/head labels present and differing refs. On any diff failure,buildComparisonResultreturns a wrappedErrCouldntComputeDiff, andGetComparisonResultpropagates that error, aborting the entire comparison. If you ever want “best-effort” diffs (i.e., keep the main comparison result even if one diff command fails), a future refinement could downgrade these failures to per-module annotations instead of returning an error.Also applies to: 22-23, 59-69, 71-76, 87-88, 100-103, 118-125, 128-133, 136-140
174-209: generateDiff is robust; consider minor naming and operational tweaks
generateDiffdoes a solid job validating the command, wiring env vars, and returning detailed output on non-zero exit codes. Two small, optional refinements you might consider:
- Rename
baseLabel/headLabelparams tobaseRef/headRef(or similar) to reflect that they hold ref values, not labels.- If you expect long-running or potentially hanging diff commands, consider an
exec.CommandContextvariant with a timeout, so a single misbehaving command can’t stall the whole run.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.suminternal/domain/__snapshots__/config_test.snapis excluded by!**/*.snapinternal/view/__snapshots__/TestRenderHTML_works_for_built_in_template_1.snapis excluded by!**/*.snapinternal/view/__snapshots__/TestRenderHTML_works_for_built_in_template_works_when_diffs_are_present_1.snapis excluded by!**/*.snaptests/cli/__snapshots__/TestCompareModulesCmd_help_flag_works_1.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
go.mod(2 hunks)internal/cmd/compare_modules.go(3 hunks)internal/domain/config.go(3 hunks)internal/domain/config_test.go(1 hunks)internal/domain/types.go(1 hunks)internal/domain/validation.go(2 hunks)internal/services/compare_modules.go(6 hunks)internal/services/compare_modules_test.go(5 hunks)internal/view/assets/template.html(3 hunks)internal/view/html.go(1 hunks)internal/view/html_test.go(1 hunks)internal/view/stdout.go(2 hunks)internal/view/types.go(3 hunks)
🔇 Additional comments (16)
internal/domain/config.go (2)
33-37: LGTM! Clean type definition.The DiffConfig structure is well-defined with clear fields for base/head labels and the command to execute.
91-129: Solid validation and normalization logic.The parse() method correctly:
- Trims whitespace from all inputs
- Validates required fields are not empty
- Filters out empty command elements
- Accumulates and returns detailed errors
The error handling and validation are appropriate for this configuration parsing use case.
internal/domain/config_test.go (1)
10-74: Comprehensive test coverage.The test suite thoroughly covers the rawDiffConfig parsing logic including:
- Normal parsing
- Whitespace normalization
- Empty value validation
- Whitespace-only value rejection
The use of snapshot testing is appropriate for validating the parsed structure and error messages.
internal/view/html_test.go (1)
93-200: Good test coverage for diff rendering.The test properly validates that:
- DiffResult objects are correctly constructed
- Diffs are properly attached to ModuleResults
- The HTML renderer correctly processes and includes diffs in the output
The use of realistic git diff output in the test data is appropriate.
internal/view/html.go (2)
28-40: LGTM! Improved clarity.The variable rename from
moduletomoduleResultimproves readability and makes the code more explicit about what's being processed.
42-54: Note: Unescaped HTML output for diffs.The use of
template.HTML()on line 48 bypasses HTML escaping, which is necessary to render diff syntax correctly. Since the diff output originates from user-configured commands executed locally, the security risk is acceptable. However, be aware that any malicious content in the diff output would be rendered without sanitization.internal/cmd/compare_modules.go (2)
30-30: LGTM! Clear flag definition.The
--include-diffsflag is well-documented with:
- Clear purpose in the help text
- Short form
-dfor convenience- Note about requiring diffConfig in the config file
Also applies to: 194-200
116-121: Clean integration of includeDiffs parameter.The flag value is properly passed through to the service layer. The function call is clear and maintains good separation of concerns.
internal/domain/types.go (2)
37-43: LGTM! Well-designed type.The DiffResult structure appropriately captures:
- Raw diff output as []byte for binary-safe handling
- Source labels for context
- Version references (BaseRef/HeadRef) for tracking what's being compared
45-50: Clean integration into ModuleResult.The optional DiffResult pointer field allows modules to optionally include diff information without affecting existing functionality. The YAML tag with
omitemptyensures clean serialization.internal/view/assets/template.html (4)
12-16: Good conditional loading of Highlight.js.The syntax highlighting assets are only loaded when diffs are present, which avoids unnecessary resource loading when diffs are not included in the report.
74-92: Nice UX for diff exploration.The collapsible details elements with a toggle-all button provide a clean way to explore diffs without overwhelming the page. The summary line clearly shows what's being compared (module name, labels, and version refs).
114-125: Clean toggle and highlighting implementation.The JavaScript correctly:
- Toggles all details elements simultaneously
- Checks current state to decide whether to open or close
- Initializes syntax highlighting after DOM load
13-15: Version is current; CDN dependency remains an operational consideration.The template uses Highlight.js 11.11.1, which is the latest stable version. However, the operational risk of external CDN dependency remains valid: if cdnjs.cloudflare.com or fonts.googleapis.com are unavailable, the HTML report may not render correctly. For critical deployments, consider vendoring these assets or documenting the external dependencies explicitly.
go.mod (1)
6-6: I'll search for the latest versions of these dependencies to verify currency:I need to verify if v2.20.0 is the latest version of chroma:
Verify dependency versions and security advisories — versions confirmed as current with no known vulnerabilities.
Based on verification:
- Chroma v2 was published May 16, 2025, confirming v2.20.0 is current
- Regexp2 was published Dec 31, 2024, indicating active maintenance
- Security advisory queries returned no vulnerabilities for either package
- Both packages are MIT-licensed, stable dependencies with extensive adoption (chroma: 245 importers, regexp2: 932 importers)
No action required; the dependencies are production-ready.
internal/domain/validation.go (1)
104-118: DiffCfg parsing and error propagation look solidThe diff config parsing is correctly integrated: errors are folded into
comparisonErrorswith a clear, multi-line message, and a successfully parsed config is safely propagated viadiffCfgToUseinto the validatedComparison. No functional issues here.Also applies to: 128-128
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
internal/view/stdout.go(3 hunks)
🔇 Additional comments (3)
internal/view/stdout.go (3)
4-4: LGTM!The new imports are correctly used:
bytesfor buffering highlighted output,stringsfor efficient string assembly, andchroma/quickfor diff syntax highlighting.Also applies to: 8-8, 10-10
16-16: LGTM!The error variable rename better reflects the function's expanded scope, which now includes both table and diff rendering.
111-119: "terminal16" formatter is valid; "native" style validity unclear.The "terminal16" formatter is registered and valid in chroma v2. However, the "native" style parameter could not be verified against documented valid style names. Consider using a documented style (e.g., "monokai", "dracula", "github") or confirming "native" exists in your chroma version, since the graceful error handling fallback only applies if highlighting fails entirely.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/services/compare_modules.go (2)
102-128: Solid diff generation logic with good guards.The implementation correctly:
- Gates diff computation to out-of-sync modules with valid config
- Skips identical refs (line 107 check)
- Wraps errors with helpful context
- Only creates DiffResult when output is non-empty
However, there's a parameter naming inconsistency:
generateDiffis called withbaseRefandheadRef(lines 110-111), but the function signature uses parameter namesbaseLabelandheadLabel(line 176). The values being passed are refs (version strings/commit hashes), not labels (like "staging" or "production").Consider renaming the
generateDiffparameters frombaseLabel, headLabeltobaseRef, headRefto accurately reflect that they receive ref values, not label values:-func generateDiff(moduleName, baseLabel, headLabel string, command []string) ([]byte, error) { +func generateDiff(moduleName, baseRef, headRef string, command []string) ([]byte, error) { var zero []byte if len(command) == 0 { return zero, fmt.Errorf("empty command") } cmd := exec.Command(command[0], command[1:]...) cmd.Env = append(os.Environ(), - fmt.Sprintf("TFLENS_DIFF_BASE_REF=%s", baseLabel), - fmt.Sprintf("TFLENS_DIFF_HEAD_REF=%s", headLabel), + fmt.Sprintf("TFLENS_DIFF_BASE_REF=%s", baseRef), + fmt.Sprintf("TFLENS_DIFF_HEAD_REF=%s", headRef), fmt.Sprintf("TFLENS_DIFF_MODULE_NAME=%s", moduleName), )
176-211: Add timeout to prevent indefinite hangs.The external command execution lacks a timeout, which could cause the application to hang indefinitely if the diff command stalls or takes too long.
Use
exec.CommandContextwith a timeout to ensure the command completes within a reasonable time:-func generateDiff(moduleName, baseLabel, headLabel string, command []string) ([]byte, error) { +func generateDiff(moduleName, baseRef, headRef string, command []string) ([]byte, error) { var zero []byte if len(command) == 0 { return zero, fmt.Errorf("empty command") } - cmd := exec.Command(command[0], command[1:]...) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + cmd := exec.CommandContext(ctx, command[0], command[1:]...) cmd.Env = append(os.Environ(), - fmt.Sprintf("TFLENS_DIFF_BASE_REF=%s", baseLabel), - fmt.Sprintf("TFLENS_DIFF_HEAD_REF=%s", headLabel), + fmt.Sprintf("TFLENS_DIFF_BASE_REF=%s", baseRef), + fmt.Sprintf("TFLENS_DIFF_HEAD_REF=%s", headRef), fmt.Sprintf("TFLENS_DIFF_MODULE_NAME=%s", moduleName), )Note: This also includes the parameter renaming suggested in the previous comment. You'll need to add
"context"and"time"imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
internal/services/compare_modules.go(6 hunks)
🔇 Additional comments (5)
internal/services/compare_modules.go (5)
3-15: LGTM! Imports support the new diff functionality.The added imports are necessary for executing external diff commands and handling their output.
17-18: LGTM! Good use of sentinel error.This error constant provides a typed error for diff computation failures, enabling callers to distinguish diff errors from other failures.
19-23: LGTM! Clear API extension.The
includeDiffsparameter provides explicit control over diff computation, keeping the feature opt-in.
59-68: LGTM! Proper gating of diff configuration.The logic correctly ensures that
diffCfgis only populated whenincludeDiffsis true, effectively gating the diff computation feature.
71-76: LGTM! Signature extended appropriately.The
diffCfgparameter uses a pointer to allow nil, making diff computation optional.