add html output #8
Conversation
|
Warning Rate limit exceeded@dhth has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds HTML report generation to the compare-modules command (new flags: html-template, html-output, html-title), file I/O and timestamp handling for rendering and writing reports, and new HTML view types (HTMLConfig, HTMLData, HTMLRow). Implements view.RenderHTML with template selection, parsing, rendering and new view errors plus a default embedded HTML template asset. Replaces ModuleStatus.StatusUnknown with StatusNotApplicable and updates status logic. Adds IsErrorUnexpected and an unexpected-error branch in main. Includes HTML rendering tests and marks Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/view/assets/template.html (1)
54-58: Minor: scroll behavior may have limited browser support.The scroll button uses
behavior: 'instant', which may not be supported in older browsers (though'instant'is more widely supported than'smooth'). This is a minor issue as it degrades gracefully.internal/view/html_test.go (1)
171-243: Consider clarifying test names.The test names "works when missing modules are ignored" (line 171) and "works when missing modules are not ignored" (line 208) don't clearly convey what behavior is being tested. The main difference between these tests is the
Statusfield (StatusInSyncvsStatusOutOfSync), but the term "ignored" doesn't directly relate to this distinction.Consider renaming to better describe the expected behavior, such as:
- "works when modules with missing values have StatusInSync"
- "works when modules with missing values have StatusOutOfSync"
internal/view/html.go (1)
46-46: Consider removing unnecessary zero value declaration.The declaration
var zero stringis unnecessary since the zero value for string is already"". You can directly use""or simply omit the second return value name in error cases.Apply this diff:
- var zero string - if config.CustomTemplate != nil { tmpl, templErr = template.New("custom").Parse(*config.CustomTemplate) if templErr != nil { - return zero, fmt.Errorf("%w: %w", errCouldntParseCustomTemplate, templErr) + return "", fmt.Errorf("%w: %w", errCouldntParseCustomTemplate, templErr) } } else { tmpl, templErr = template.New("built-in").Parse(builtInTemplate) if templErr != nil { - return zero, fmt.Errorf("%w: %w", ErrCouldntParseBuiltInTemplate, templErr) + return "", fmt.Errorf("%w: %w", ErrCouldntParseBuiltInTemplate, templErr) } } var buf bytes.Buffer err := tmpl.Execute(&buf, htmlData) if err != nil { - return zero, fmt.Errorf("%w: %w", errCouldntPopulateTemplate, err) + return "", fmt.Errorf("%w: %w", errCouldntPopulateTemplate, err) }
📜 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 (7)
internal/view/__snapshots__/TestRenderHTML_works_for_all_in_sync_modules_1.snapis excluded by!**/*.snapinternal/view/__snapshots__/TestRenderHTML_works_for_built_in_template_1.snapis excluded by!**/*.snapinternal/view/__snapshots__/TestRenderHTML_works_when_missing_modules_are_ignored_1.snapis excluded by!**/*.snapinternal/view/__snapshots__/TestRenderHTML_works_when_missing_modules_are_not_ignored_1.snapis excluded by!**/*.snapinternal/view/__snapshots__/TestRenderHTML_works_when_modules_are_out_of_sync_1.snapis excluded by!**/*.snapinternal/view/__snapshots__/TestRenderHTML_works_when_only_one_label_is_present_1.snapis excluded by!**/*.snaptests/cli/__snapshots__/TestCompareModulesCmd_help_flag_works_1.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
internal/cmd/compare_modules.go(4 hunks)internal/cmd/errors.go(1 hunks)internal/domain/types.go(2 hunks)internal/services/compare_modules.go(1 hunks)internal/view/assets/template.html(1 hunks)internal/view/html.go(1 hunks)internal/view/html_test.go(1 hunks)internal/view/stdout_test.go(1 hunks)internal/view/types.go(1 hunks)main.go(1 hunks)
🔇 Additional comments (16)
main.go (1)
25-30: LGTM! Good user experience improvement.The unexpected error handling provides clear guidance for users to report issues, which will help with debugging and issue triage.
internal/view/stdout_test.go (1)
171-171: LGTM! Status change aligns with domain refactor.The change from
StatusUnknowntoStatusNotApplicableis semantically more accurate for modules with only one label present, as there's nothing to compare for sync status.Also applies to: 178-178
internal/services/compare_modules.go (1)
105-106: LGTM! More semantically accurate status.Returning
StatusNotApplicablefor modules with zero or one non-empty versions is clearer thanStatusUnknown, as there's nothing to compare for sync status.internal/view/assets/template.html (1)
6-11: Consider documenting the external CDN dependencies.The template relies on external CDN resources (Tailwind CSS and Google Fonts), which means the HTML report requires internet access to render properly. This could be a limitation for users who need offline viewing or are in restricted network environments.
Consider documenting this requirement in the command help text or README, or offering guidance on how users can create custom templates with embedded/local resources if offline viewing is needed.
internal/domain/types.go (2)
11-22: Note: String() output format has changed (potential breaking change).The status strings have changed from hyphenated format ("in-sync", "out-of-sync") to underscored format ("in_sync", "out_of_sync", "not_applicable"). If any external tools or scripts parse the stdout output and depend on these string values, this is a breaking change.
If this is purely for internal use and the HTML template, this is fine. Otherwise, consider whether this warrants a version bump or migration note.
8-8: LGTM! StatusNotApplicable is more semantically accurate.The refactor from
StatusUnknowntoStatusNotApplicablebetter represents the state when a module has insufficient data for comparison, and the symbol mapping to "-" is appropriate.Also applies to: 30-31
internal/view/types.go (1)
5-27: LGTM! Clean and well-structured types for HTML rendering.The types are appropriately designed:
CustomTemplateas*stringallows nil to indicate no custom template- Timestamp format is reasonable and consistent (UTC)
- Structure supports the template rendering needs effectively
internal/cmd/compare_modules.go (1)
17-22: HTML output implementation looks solid overall.The implementation properly handles:
- Custom template file reading with descriptive errors
- Directory creation with appropriate permissions (0o755)
- File writing with appropriate permissions (0o644)
- Comprehensive error wrapping
The default output path will overwrite existing files without warning, but this is likely expected behavior for a report generation command.
Also applies to: 132-161, 194-213
internal/cmd/errors.go (1)
9-11: Consider whether other internal errors should be classified as unexpected.The review comment raises a valid concern. Verification found that the view package contains additional internal errors beyond
ErrCouldntParseBuiltInTemplate:
errCouldntParseCustomTemplate(private)errCouldntPopulateTemplate(private)These represent programming failures similar to the built-in template parsing error. However, they are currently private and cannot be checked by
IsErrorUnexpectedwithout being exported. You should evaluate whether these internal errors should be exposed and added to the unexpected error classification, or whether the current minimal approach (checking only the exported template error) aligns with your design intent.internal/view/html_test.go (3)
1-11: LGTM!Package declaration and imports are appropriate for the test file.
13-41: LGTM!Test setup with custom template and reference time is well-structured for deterministic testing.
245-321: LGTM!The test cases for
StatusNotApplicableand error handling are well-structured and provide good coverage.internal/view/html.go (4)
1-12: LGTM!Package declaration and imports are appropriate for HTML rendering functionality.
14-21: LGTM!The embedded template and error declarations are correctly structured. The exported
ErrCouldntParseBuiltInTemplateallows external packages to check for built-in template parsing failures, while custom template errors remain internal.
28-41: Verify empty string handling for missing module values.Line 35 accesses
module.Values[label]without checking if the key exists. When a label is missing from the map, Go returns the zero value (empty string). Based on the test cases, this appears intentional for displaying missing values, but please confirm this is the expected behavior for the HTML output.
48-67: LGTM!The template parsing and execution logic is well-structured with proper error wrapping. The conditional handling of custom vs built-in templates is clear and correct.
No description provided.