Glamorous implementation #853
Conversation
mergify
bot
added
triage
Cerebrovinny
force-pushed
the
glamour-implementation
branch
from
270e04a to
cb4fa84
Compare
Cerebrovinny
force-pushed
the
glamour-implementation
branch
from
cb4fa84 to
991d653
Compare
|
💥 This pull request now has conflicts. Could you fix it @Cerebrovinny? 🙏 |
Cerebrovinny
force-pushed
the
glamour-implementation
branch
from
1c1837a to
3a8bc87
Compare
Cerebrovinny
force-pushed
the
glamour-implementation
branch
2 times, most recently
from
b3d16f0 to
940595d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
mergify
bot
removed
the
wip
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the Atmos CLI, focusing on the Changes
Possibly related PRs
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
pkg/ui/markdown/styles.go (3)
13-17: Avoid swallowing errors.If the user’s custom configuration fails to load (line 14), we silently fall back to built-in defaults. Consider adding a log message or an error return so the user knows their config is not being used.
33-36: Check unmarshal errors more thoroughly.We simply return an error if JSON unmarshalling fails. It might be helpful to wrap the error to clarify that it occurred during style deserialization.
94-257: Consider using a single JSON file for built-in styles.Currently, the built-in style is embedded in code. Storing it in a JSON file would make it more maintainable and consistent with the config-driven approach.
internal/tui/workflow/model.go (3)
25-25: Use consistent naming for simplicity.The field name
workflowsis a map of workflow manifests, while the first column is called “Workflow Manifests.” Consider renaming fields or titles for consistency (e.g.,workflowManifests).
152-157: Keep step item labeling consistent.As above, defaulting the name to the command is a coherent fallback. Ensure no collisions happen if multiple steps share an empty name. Could optionally generate a unique label in such a scenario.
184-189: Watch for potential repeated logic.The step item creation logic in lines 184-189 is nearly identical to lines 152-157. Centralizing this logic in a helper method helps keep it DRY.
pkg/schema/schema.go (2)
624-641: Double-check naming consistency.Fields like
BlockPrefixandPrefixcan confuse usage if not well-documented. Clarify if both are required, or unify them for clarity.
643-645: Potential for extended syntax highlighting.Your
ChromaStylecurrently stores color alone. If you plan to theme bold or italics for code tokens, consider adding extra fields or using the advanced Chroma config directly.cmd/markdown/workflow.md (4)
1-2: Document valid commands more explicitly.You reference “
atmos workflow listis not valid.” Possibly provide a quick link to the valid commands for immediate clarity beyond your examples.
20-20: Consider consistent punctuation or an em dash.To match style guidelines, consider an em dash instead of the lone hyphen when introducing your enumerations (e.g., “– Deploy a workflow” -> “— Deploy a workflow”). This minor detail can enhance readability.
🧰 Tools
🪛 Markdownlint (0.37.0)
20-20: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
38-39: Inconsistent messaging.Line 39 references “Execute a specific workflow” after showing the command to list directory contents. Consider re-checking the heading or examples ordering to ensure clarity.
🧰 Tools
🪛 LanguageTool
[typographical] ~39-~39: Consider using an em dash in dialogues and enumerations.
Context: ... workflowsshell $ ls workflows/– Execute a specific workflow ```shell $ ...(DASH_RULE)
🪛 Markdownlint (0.37.0)
38-38: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
7-11: Markdownlint reminder: removing$from prompts.Static analysis suggests removing
$to avoid style checks (MD014). Possibly keep the$for clarity, or remove them consistently across all examples.Also applies to: 20-21, 24-25, 28-29, 38-39, 42-42
🧰 Tools
🪛 Markdownlint (0.37.0)
7-7: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
8-8: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
9-9: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
10-10: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
internal/tui/components/code_view/main.go (1)
40-59: Consider error handling improvements in the SetContent method.While the implementation looks good, the error handling could be enhanced to provide more context about why the rendering failed.
Consider this improvement:
if language == "markdown" || language == "md" { m.IsMarkdown = true rendered, err = u.RenderMarkdown(content, "") if err != nil { - // Fallback to plain text if markdown rendering fails + // Log the error for debugging purposes + log.Printf("markdown rendering failed: %v", err) + // Fallback to plain text rendered = content } } else { m.IsMarkdown = false rendered, err = u.HighlightCode(content, language, m.SyntaxTheme) if err != nil { - // Fallback to plain text if syntax highlighting fails + // Log the error for debugging purposes + log.Printf("syntax highlighting failed for language %s: %v", language, err) + // Fallback to plain text rendered = content } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/markdown/workflow.md(1 hunks)cmd/workflow.go(1 hunks)go.mod(1 hunks)internal/tui/components/code_view/main.go(3 hunks)internal/tui/utils/utils.go(2 hunks)internal/tui/workflow/model.go(5 hunks)pkg/schema/schema.go(2 hunks)pkg/ui/markdown/colors.go(1 hunks)pkg/ui/markdown/parser.go(1 hunks)pkg/ui/markdown/renderer.go(1 hunks)pkg/ui/markdown/styles.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
cmd/markdown/workflow.md
[typographical] ~21-~21: Consider using an em dash in dialogues and enumerations.
Context: ...kflow deploy-infra --file workflow1 ``` – Deploy with stack configuration ...
(DASH_RULE)
[typographical] ~25-~25: Consider using an em dash in dialogues and enumerations.
Context: ...-infra --file workflow1 --stack dev ``` – Resume from a specific step ...
(DASH_RULE)
[uncategorized] ~36-~36: Possible missing preposition found.
Context: ...could not be found. ## Examples – List available workflows shell $ ls workflows/ ...
(AI_HYDRA_LEO_MISSING_OF)
[typographical] ~39-~39: Consider using an em dash in dialogues and enumerations.
Context: ... workflows shell $ ls workflows/ – Execute a specific workflow ```shell $ ...
(DASH_RULE)
🪛 Markdownlint (0.37.0)
cmd/markdown/workflow.md
7-7: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
8-8: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
9-9: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
10-10: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
20-20: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
24-24: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
28-28: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
38-38: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
42-42: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
🔇 Additional comments (35)
cmd/workflow.go (12)
4-7: Imports look correct.
They provide the required functionalities for embedding resources and handling strings.
12-12: New import for markdown package is fine.
This integration allows seamless rendering of markdown-based error messages and usage guides.
16-18: Using Go embed to load the workflow markdown file is a good practice.
This approach ensures that the markdown content is packaged efficiently with the binary.
19-24: Structured error message definition is concise and clear.
This provides an excellent foundation for rendering detailed user-facing error messages.
26-42: Error rendering function is straightforward and well-structured.
Nice job returning wrapped errors whenever an internal step fails.
44-55: Good separation of concerns in getMarkdownSection.
Extracting content from the embedded markdown fosters maintainable and modular code.
61-67: Enhanced usage examples are helpful.
Clarifying the use of --file, --stack, and --from-step flags improves user education.
71-79: Graceful handling of zero-argument scenario.
Invoking ExecuteWorkflowCmd with no arguments and logging errors fosters a better user experience.
80-92: Excellent approach to invalid command handling.
Rendering a custom error message for list helps guide the user toward valid workflow subcommands.
94-107: Clear check for the required --file flag.
Providing a dedicated error message offers immediate feedback to users about missing input.
109-138: Robust error categorization.
It’s great that you are parsing known errors to offer more contextual suggestions while still preserving a generic fallback for unknown errors.
145-148: Flag definitions are consistent and descriptive.
The usage messages align well with the usage examples above.
internal/tui/utils/utils.go (2)
9-9: New import for glamour is appropriate.
It powers rendering of markdown content within terminals.
31-50: RenderMarkdown function is well-structured.
It gracefully defaults to the “dark” style and uses an 80-character word wrap for readability.
pkg/ui/markdown/colors.go (1)
1-28: Color definitions align with the brand and ensure consistent rendering.
The usage of lipgloss.AdaptiveColor for light/dark modes is an excellent choice for user accessibility.
pkg/ui/markdown/parser.go (2)
7-35: ParseMarkdownSections is a simple, effective approach for splitting markdown content by headings.
It neatly accumulates lines for each section before moving on to the next.
37-55: SplitMarkdownContent uses double newlines to split details and suggestions.
The logic is straightforward and should handle typical docstring formats well.
pkg/ui/markdown/renderer.go (8)
1-16: Introduction of the Renderer struct is a sound approach.
It neatly encapsulates rendering logic and associated rendering parameters.
18-50: NewRenderer function effectively assembles the renderer with optional configurations.
This pattern is flexible for future expansions of rendering capabilities.
52-55: Generic Render method is straightforward.
It provides a base function for all subsequent specialized rendering scenarios.
57-71: RenderWithStyle supports dynamic theming at runtime.
This opens the door for user-specified style overrides without altering the core application.
73-78: RenderWorkflow is a nice specialized extension of Render.
It introduces a dedicated heading, improving the approachability of workflow documentation.
80-101: RenderError elegantly formats errors with optional sections for examples.
This helps highlight usage guidance while preserving the error’s context.
103-112: RenderSuccess parallels RenderError for positive feedback messages.
A clear, consistent message structure fosters a good user experience.
114-129: The Option pattern is well-adopted.
Allowing configuration through closures is a canonical way to keep the code flexible.
pkg/ui/markdown/styles.go (2)
38-78: Ensure partial overrides are cohesive.
When selectively overriding style fields, confirm that partial overrides (e.g., only color but not background) integrate properly with other fields. It may be beneficial to confirm that no overshadowed or unexpected styles remain.
259-269: Pointer helpers look good.
The helper functions for string, bool, and uint pointers clarify the pointer usage for style config. This approach is straightforward and maintainable.
internal/tui/workflow/model.go (2)
89-94: Graceful fallback for empty step names.
Great job defaulting to s.Command if s.Name is empty. This ensures usability when a workflow step is missing a name. Keep verifying if all user-provided commands are non-empty.
236-238: Init method is minimal but correct.
The Init method properly returns nil. We might consider delivering a quick hint to the user if no workflows are loaded, but this is optional.
pkg/schema/schema.go (2)
41-43: Future proof settings with optional fields.
You’ve added Markdown to AtmosSettings. If there’s a scenario where no markdown styles are needed at all, consider clarifying whether empty structures or nil references are fully supported.
592-622: Granular customization is well-structured.
The MarkdownSettings structure is carefully separated by element. This design offers significant flexibility and is straightforward to update for future styling additions.
internal/tui/components/code_view/main.go (3)
15-15: LGTM! Good addition of the markdown flag.
The IsMarkdown boolean field is a clean way to track the content type.
29-29: LGTM! Proper initialization of the new field.
The IsMarkdown field is correctly initialized to false in the New function.
67-67: Verify the rendered content's width and height constraints.
The viewport's content is being rendered with the viewport's dimensions, but we should ensure that the rendered markdown or highlighted code respects these constraints.
✅ Verification successful
Width and height constraints are properly handled, warrior! 💪
The code is already enforcing viewport dimensions correctly. Looking at the search results, we can see that:
- The viewport is initialized with proper dimensions in
internal/tui/components/code_view/main.go - The rendered content is constrained using the viewport's dimensions through lipgloss styling:
Width(m.Viewport.Width).
Height(m.Viewport.Height).
Render(rendered)This implementation ensures that both markdown and highlighted code content respect the viewport's constraints. The content will be properly contained within the viewport's boundaries.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing viewport dimension handling
ast-grep --pattern 'func ($_, $_) SetSize(width int, height int) {
$$$
style := lipgloss.NewStyle().Width($_).Height($_)
$$$
}'
Length of output: 135
Script:
#!/bin/bash
# Let's search for viewport-related code and dimension handling
rg -A 5 "viewport" --type go
# Also search for any width/height style settings
rg -A 3 "Width\(|Height\(" --type go
Length of output: 5379
go.mod (1)
42-43: Verify compatibility with the updated dependencies.
The changes include:
- Addition of
github.com/muesli/termenvwith a specific pre-release version - Update of
github.com/open-policy-agent/opafrom v1.0.0 to v0.70.0
These are significant version changes that warrant testing.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (14)
cmd/markdown/workflow.md (2)
3-17: Enhance command examples with descriptions and use cases.The examples are clear but could be more helpful with additional context. Consider adding:
- Brief description of what each command accomplishes
- Example values for placeholders (e.g., what typical values for
<workflow-name>look like)- Expected outcomes for each command
– Use interactive UI + # Opens an interactive terminal UI for workflow selection and execution $ atmos workflow – Execute a workflow + # Executes a specific workflow from a file + # Example: atmos workflow deploy --file workflows/deploy.yaml $ atmos workflow <workflow-name> --file <file>🧰 Tools
🪛 LanguageTool
[typographical] ~3-~3: Consider using an em dash in dialogues and enumerations.
Context: Examples: – Use interactive UI $ atmos workflo...(DASH_RULE)
[typographical] ~7-~7: Consider using an em dash in dialogues and enumerations.
Context: ...interactive UI $ atmos workflow – Execute a workflow $ atmos workflo...(DASH_RULE)
[typographical] ~11-~11: Consider using an em dash in dialogues and enumerations.
Context: ...rkflow --file – Execute with stack override $ atmo...(DASH_RULE)
[typographical] ~15-~15: Consider using an em dash in dialogues and enumerations.
Context: ...-name> --file --stack – Resume from specific step $ atmos ...(DASH_RULE)
🪛 Markdownlint (0.37.0)
5-5: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
9-9: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
13-13: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
17-17: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
19-20: Convert bare URL to markdown link format.For better markdown consistency and potential future styling:
-For more information, refer to the **docs**: -https://atmos.tools/cli/commands/workflow/ +For more information, refer to the [**documentation**](https://atmos.tools/cli/commands/workflow/)🧰 Tools
🪛 Markdownlint (0.37.0)
20-20: null
Bare URL used(MD034, no-bare-urls)
internal/tui/utils/utils.go (1)
41-45: Consider caching the default style.The style is fetched on every render call, which could be inefficient for repeated renders.
Consider implementing a style cache:
+var defaultStyleCache []byte +var defaultStyleErr error + func RenderMarkdown(markdownText string, style string) (string, error) { if markdownText == "" { return "", fmt.Errorf("empty markdown input") } - // Get the custom style from atmos config - customStyle, err := mdstyle.GetDefaultStyle() + // Use cached style or fetch if not cached + var customStyle []byte + if defaultStyleCache == nil { + defaultStyleCache, defaultStyleErr = mdstyle.GetDefaultStyle() + } + customStyle, err := defaultStyleCache, defaultStyleErrinternal/tui/components/code_view/main.go (2)
43-49: Enhance markdown detection and error logging.The current markdown detection is basic and error handling silently falls back.
Consider:
- Using file extension or content analysis for more accurate detection
- Logging render errors for debugging
- if language == "markdown" || language == "md" { + if isMarkdownContent(language, content) { m.IsMarkdown = true rendered, err = u.RenderMarkdown(content, "") if err != nil { + log.Printf("markdown rendering failed: %v", err) // Fallback to plain text if markdown rendering fails rendered = content }
59-67: Consider content caching for performance.The content is re-rendered on every viewport update.
Consider caching the rendered content when the input hasn't changed:
+ // Only update if content has changed + if rendered != m.HighlightedContent { m.HighlightedContent = rendered + } m.Viewport.ViewUp() m.Viewport.MouseWheelEnabled = truecmd/workflow.go (1)
160-184: Refactor error handling to reduce duplication.The error handling logic contains repeated patterns that could be simplified using a map-based approach.
- if strings.Contains(err.Error(), "does not exist") { - details, suggestion := getMarkdownSection("Workflow File Not Found") - err := renderError(ErrorMessage{ - Title: "Workflow File Not Found", - Details: details, - Suggestion: suggestion, - }) - if err != nil { - u.LogErrorAndExit(schema.AtmosConfiguration{}, err) - } - } else if strings.Contains(err.Error(), "does not have the") { - details, suggestion := getMarkdownSection("Invalid Workflow") - err := renderError(ErrorMessage{ - Title: "Invalid Workflow", - Details: details, - Suggestion: suggestion, - }) - if err != nil { - u.LogErrorAndExit(schema.AtmosConfiguration{}, err) - } + errorMapping := map[string]string{ + "does not exist": "Workflow File Not Found", + "does not have the": "Invalid Workflow", + } + + for errText, title := range errorMapping { + if strings.Contains(err.Error(), errText) { + details, suggestion := getMarkdownSection(title) + if err := renderError(ErrorMessage{ + Title: title, + Details: details, + Suggestion: suggestion, + }); err != nil { + u.LogErrorAndExit(schema.AtmosConfiguration{}, err) + } + return + } + }pkg/ui/markdown/styles.go (1)
98-264: Consider extracting color constants to a dedicated package.The color constants are used throughout the style configuration. Consider moving them to a dedicated colors package for better maintainability.
Create a new package
pkg/ui/colorswith:package colors const ( White = "#FFFFFF" Purple = "#9B51E0" Blue = "#00A3E0" )website/docs/cli/configuration/markdown-styling.mdx (4)
157-171: Update default theme colors to use semantic color variables.The hardcoded colors in the default theme should be replaced with semantic color variables for better maintainability and consistency.
document: - color: "#FFFFFF" # White text + color: "${colors.text}" # Default text color heading: - color: "#00A3E0" # Blue headings + color: "${colors.primary}" # Primary color for headings bold: true h1: - color: "#FFFFFF" # White text - background_color: "#9B51E0" # Purple background + color: "${colors.primary}" # Primary color + background_color: "${colors.background}" # Default background bold: true margin: 2 code_block: - color: "#00A3E0" # Blue code + color: "${colors.secondary}" # Secondary color for code margin: 1 link: - color: "#00A3E0" # Blue links + color: "${colors.primary}" # Primary color for links
184-184: Add missing article in sentence.-Automatically maps hex colors to nearest ANSI 256 colors +Automatically maps hex colors to the nearest ANSI 256 colors🧰 Tools
🪛 LanguageTool
[grammar] ~184-~184: A determiner may be missing.
Context: ...** - Automatically maps hex colors to nearest ANSI 256 colors - Detected via `$TERM...(THE_SUPERLATIVE)
198-198: Use em dash for better readability.-You don't need to configure anything - your styles will work everywhere +You don't need to configure anything — your styles will work everywhere🧰 Tools
🪛 LanguageTool
[typographical] ~198-~198: To join two clauses or introduce examples, consider using an em dash.
Context: ...on. You don't need to configure anything - your styles will work everywhere, automa...(DASH_RULE)
267-267: Add missing period after abbreviation.-Terminal.app, iTerm2, Windows Terminal, etc) +Terminal.app, iTerm2, Windows Terminal, etc.)🧰 Tools
🪛 LanguageTool
[style] ~267-~267: In American English, abbreviations like “etc.” require a period.
Context: ...Terminal.app, iTerm2, Windows Terminal, etc). 3. **Configure Environment Variables...(ETC_PERIOD)
website/docs/cli/configuration/configuration.mdx (1)
204-216: Add migration guide for terminal settings.Add a migration guide to help users transition from the deprecated
docssection to the newterminalsettings.Add the following section before the deprecated notice:
### Migrating from docs to terminal settings If you're using the deprecated `docs` section, migrate your settings as follows: | Old Setting | New Setting | |------------|-------------| | `docs.max-width` | `terminal.max_width` | | `docs.pagination` | `terminal.pager` | The new `terminal` section provides additional features like timestamps, colors, and unicode support.pkg/schema/schema.go (2)
635-652: Consider adding color validation and documentationWhile the MarkdownStyle structure is well-designed, consider these enhancements:
- Add field documentation for color format (e.g., "hex: #RRGGBB", "name: red")
- Consider adding validation for color values
Add documentation comments to clarify color usage:
type MarkdownStyle struct { + // Color supports hex (#RRGGBB) or named colors (e.g., "red", "blue") Color string `yaml:"color,omitempty" json:"color,omitempty" mapstructure:"color"` + // BackgroundColor supports hex (#RRGGBB) or named colors (e.g., "red", "blue") BackgroundColor string `yaml:"background_color,omitempty" json:"background_color,omitempty" mapstructure:"background_color"`
654-656: Consider expanding ChromaStyle capabilitiesThe ChromaStyle structure currently only supports color. Consider adding more styling options like bold, italic, or background color for richer syntax highlighting capabilities.
type ChromaStyle struct { Color string `yaml:"color,omitempty" json:"color,omitempty" mapstructure:"color"` + BackgroundColor string `yaml:"background_color,omitempty" json:"background_color,omitempty" mapstructure:"background_color"` + Bold bool `yaml:"bold,omitempty" json:"bold,omitempty" mapstructure:"bold"` + Italic bool `yaml:"italic,omitempty" json:"italic,omitempty" mapstructure:"italic"` }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
atmos.yaml(1 hunks)cmd/docs.go(2 hunks)cmd/markdown/workflow.md(1 hunks)cmd/workflow.go(1 hunks)go.mod(4 hunks)internal/tui/components/code_view/main.go(3 hunks)internal/tui/utils/utils.go(2 hunks)internal/tui/workflow/model.go(5 hunks)pkg/schema/schema.go(2 hunks)pkg/ui/markdown/colors.go(1 hunks)pkg/ui/markdown/parser.go(1 hunks)pkg/ui/markdown/renderer.go(1 hunks)pkg/ui/markdown/styles.go(1 hunks)website/docs/cli/configuration/configuration.mdx(3 hunks)website/docs/cli/configuration/markdown-styling.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- go.mod
- cmd/docs.go
- pkg/ui/markdown/colors.go
- internal/tui/workflow/model.go
- pkg/ui/markdown/parser.go
🧰 Additional context used
🪛 LanguageTool
website/docs/cli/configuration/markdown-styling.mdx
[grammar] ~184-~184: A determiner may be missing.
Context: ...** - Automatically maps hex colors to nearest ANSI 256 colors - Detected via `$TERM...
(THE_SUPERLATIVE)
[typographical] ~198-~198: To join two clauses or introduce examples, consider using an em dash.
Context: ...on. You don't need to configure anything - your styles will work everywhere, automa...
(DASH_RULE)
[style] ~267-~267: In American English, abbreviations like “etc.” require a period.
Context: ...Terminal.app, iTerm2, Windows Terminal, etc). 3. **Configure Environment Variables...
(ETC_PERIOD)
cmd/markdown/workflow.md
[typographical] ~3-~3: Consider using an em dash in dialogues and enumerations.
Context: Examples: – Use interactive UI $ atmos workflo...
(DASH_RULE)
[typographical] ~7-~7: Consider using an em dash in dialogues and enumerations.
Context: ...interactive UI $ atmos workflow – Execute a workflow $ atmos workflo...
(DASH_RULE)
[typographical] ~11-~11: Consider using an em dash in dialogues and enumerations.
Context: ...rkflow --file – Execute with stack override $ atmo...
(DASH_RULE)
[typographical] ~15-~15: Consider using an em dash in dialogues and enumerations.
Context: ...-name> --file --stack – Resume from specific step $ atmos ...
(DASH_RULE)
🪛 golangci-lint (1.62.2)
cmd/workflow.go
96-96: Error return value of cmd.Help is not checked
(errcheck)
🪛 Markdownlint (0.37.0)
cmd/markdown/workflow.md
5-5: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
9-9: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
13-13: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
17-17: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
20-20: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (8)
pkg/ui/markdown/renderer.go (2)
105-127: Well-structured URL deduplication logic!The URL deduplication logic is cleanly implemented, improving readability by preventing duplicate links.
111-112:⚠️ Potential issueCritical: Undefined constant
Purpleused in styling.The code references an undefined
Purpleconstant. This will cause compilation errors.Add the constant definition at the package level:
+const ( + Purple = "5" // ANSI color code for purple +)Likely invalid or redundant comment.
cmd/workflow.go (1)
76-83: Well-structured command examples!The command examples are clear, concise, and follow a consistent format.
pkg/ui/markdown/styles.go (1)
11-21: Excellent null pointer safety handling!The
applyStyleSafelyfunction properly handles nil pointers, preventing potential runtime panics.website/docs/cli/configuration/markdown-styling.mdx (1)
61-66: 🛠️ Refactor suggestionConsolidate console settings under terminal section.
The console settings should be moved under the terminal section for better organization and to avoid redundancy.
- # Console output configuration - console: - pager: true # Enable pager for long output - timestamps: false # Show timestamps in logs - colors: true # Enable colored output - unicode: true # Use unicode characters + terminal: + pager: true # Enable pager for long output + timestamps: false # Show timestamps in logs + colors: true # Enable colored output + unicode: true # Use unicode charactersLikely invalid or redundant comment.
pkg/schema/schema.go (3)
40-46: Well-structured terminal configuration type!The
Terminaltype is cleanly implemented with appropriate field types and consistent tag naming.
49-52: Clean integration of new settings fields!The new Terminal and Markdown fields are well-integrated into the AtmosSettings structure, maintaining consistency with existing patterns.
604-633: Comprehensive markdown element coverage!The MarkdownSettings structure provides excellent coverage of markdown elements, making it highly flexible for various styling needs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/workflow.go (1)
162-186: 🛠️ Refactor suggestionSimplify error handling logic
The error handling section contains duplicated code that could be simplified using a map-based approach.
This suggestion was previously made and is still valid. The map-based approach would make the code more maintainable and easier to extend with new error types.
🧹 Nitpick comments (9)
website/docs/cli/configuration/configuration.mdx (1)
159-163: Consider adding more terminal configuration examples.While the basic terminal settings are documented well, it would be helpful to include examples showing different combinations of settings and their effects.
Consider adding examples like:
terminal: # Example 1: Compact output max_width: 80 pager: false # Example 2: Wide output with paging max_width: 160 pager: truecmd/workflow.go (4)
29-57: Consider extracting terminal width calculation logicThe terminal width calculation logic is duplicated in multiple places. Consider extracting it into a helper function for better maintainability.
+func getTerminalWidth(atmosConfig schema.AtmosConfiguration) uint { + termWriter := termwriter.NewResponsiveWriter(os.Stdout) + screenWidth := termWriter.(*termwriter.TerminalWriter).GetWidth() + + if atmosConfig.Settings.Docs.MaxWidth > 0 { + screenWidth = uint(min(atmosConfig.Settings.Docs.MaxWidth, int(screenWidth))) + } + return screenWidth +} func renderError(msg ErrorMessage) error { atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) if err != nil { return fmt.Errorf("failed to initialize atmos config: %w", err) } - termWriter := termwriter.NewResponsiveWriter(os.Stdout) - screenWidth := termWriter.(*termwriter.TerminalWriter).GetWidth() - - if atmosConfig.Settings.Docs.MaxWidth > 0 { - screenWidth = uint(min(atmosConfig.Settings.Docs.MaxWidth, int(screenWidth))) - } + screenWidth := getTerminalWidth(atmosConfig)
59-70: Enhance error handling in getMarkdownSectionThe function could benefit from more robust error handling when sections or parts are not found.
func getMarkdownSection(title string) (details, suggestion string) { sections := markdown.ParseMarkdownSections(workflowMarkdown) if section, ok := sections[title]; ok { parts := markdown.SplitMarkdownContent(section) if len(parts) >= 2 { return parts[0], parts[1] } + // Log a warning when suggestion part is missing + u.LogWarning(fmt.Sprintf("No suggestion found for section: %s", title)) return section, "" } + u.LogWarning(fmt.Sprintf("Section not found: %s", title)) return "", "" }
76-83: Consider adding more workflow examplesThe examples are clear and follow a good progression. Consider adding examples for common use cases like:
- Using multiple steps in a workflow
- Error handling scenarios
- Using environment variables
194-197: Consider adding flag validationWhile the flags are well-defined, consider adding validation to ensure that required combinations of flags are provided and values are within expected ranges.
func init() { workflowCmd.DisableFlagParsing = false workflowCmd.PersistentFlags().StringP("file", "f", "", "atmos workflow <name> --file <file>") workflowCmd.PersistentFlags().Bool("dry-run", false, "atmos workflow <name> --file <file> --dry-run") workflowCmd.PersistentFlags().StringP("stack", "s", "", "atmos workflow <name> --file <file> --stack <stack>") workflowCmd.PersistentFlags().String("from-step", "", "atmos workflow <name> --file <file> --from-step <step-name>") + + // Add validation for required flags + workflowCmd.MarkPersistentFlagRequired("file") + + // Add validation for flag dependencies + workflowCmd.PreRunE = func(cmd *cobra.Command, args []string) error { + fromStep, _ := cmd.Flags().GetString("from-step") + if fromStep != "" { + if len(args) == 0 { + return fmt.Errorf("workflow name is required when using --from-step") + } + } + return nil + } RootCmd.AddCommand(workflowCmd) }website/docs/cli/configuration/markdown-styling.mdx (4)
179-179: Add missing determiner-Automatically maps hex colors to nearest ANSI 256 colors +Automatically maps hex colors to the nearest ANSI 256 colors🧰 Tools
🪛 LanguageTool
[grammar] ~179-~179: A determiner may be missing.
Context: ...** - Automatically maps hex colors to nearest ANSI 256 colors - Detected via `$TERM...(THE_SUPERLATIVE)
193-193: Use em dash for better readability-You don't need to configure anything - your styles will work everywhere +You don't need to configure anything — your styles will work everywhere🧰 Tools
🪛 LanguageTool
[typographical] ~193-~193: To join two clauses or introduce examples, consider using an em dash.
Context: ...on. You don't need to configure anything - your styles will work everywhere, automa...(DASH_RULE)
262-262: Add missing period after abbreviation-Use a terminal emulator known for true color support (e.g., Terminal.app, iTerm2, Windows Terminal, etc) +Use a terminal emulator known for true color support (e.g., Terminal.app, iTerm2, Windows Terminal, etc.)🧰 Tools
🪛 LanguageTool
[style] ~262-~262: In American English, abbreviations like “etc.” require a period.
Context: ...Terminal.app, iTerm2, Windows Terminal, etc). 3. **Configure Environment Variables...(ETC_PERIOD)
226-236: Clarify relationship withatmos docscommandBased on previous discussions, we should clarify that this styling configuration is separate from the
atmos docscommand which uses Docusaurus.Add a note explaining the scope:
Atmos uses the [Glamour](https://github.com/charmbracelet/glamour) library for markdown rendering and styling. The styling is handled automatically based on your terminal's capabilities and color profile. + +> **Note:** This styling configuration applies to CLI output only. The `atmos docs` command uses Docusaurus for documentation rendering and has separate styling configuration. Key features of the markdown rendering:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/workflow.go(1 hunks)internal/tui/utils/utils.go(2 hunks)website/docs/cli/configuration/configuration.mdx(3 hunks)website/docs/cli/configuration/markdown-styling.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/utils/utils.go
🧰 Additional context used
🪛 LanguageTool
website/docs/cli/configuration/markdown-styling.mdx
[grammar] ~179-~179: A determiner may be missing.
Context: ...** - Automatically maps hex colors to nearest ANSI 256 colors - Detected via `$TERM...
(THE_SUPERLATIVE)
[typographical] ~193-~193: To join two clauses or introduce examples, consider using an em dash.
Context: ...on. You don't need to configure anything - your styles will work everywhere, automa...
(DASH_RULE)
[style] ~262-~262: In American English, abbreviations like “etc.” require a period.
Context: ...Terminal.app, iTerm2, Windows Terminal, etc). 3. **Configure Environment Variables...
(ETC_PERIOD)
🔇 Additional comments (6)
website/docs/cli/configuration/configuration.mdx (3)
108-112: LGTM! Clear and concise settings configuration example.The new settings section is well-structured and includes helpful comments explaining the configuration options.
185-198: LGTM! Well-structured terminal settings documentation.The documentation clearly explains each terminal setting with appropriate descriptions.
199-210: LGTM! Clear deprecation notice with migration path.The deprecation notice effectively communicates the change and provides clear guidance on migrating from
docstoterminalsettings.cmd/workflow.go (2)
4-20: Well-structured imports and documentation embedding!The use of
embedfor markdown documentation is a clean approach that keeps the documentation close to the code. The imports are well-organized and logically grouped.
22-27: Clean error message structure!The
ErrorMessagestruct provides a clear and organized way to represent error messages with title, details, and suggestion fields.website/docs/cli/configuration/markdown-styling.mdx (1)
143-168: 🛠️ Refactor suggestionUpdate default styles to be more generic
The current default styles make assumptions about usage (e.g., H1 with purple background) and use hardcoded colors instead of theme variables.
Apply these improvements:
# Built-in default theme settings: markdown: document: - color: "#FFFFFF" # White text + color: "${colors.text}" # Default text color heading: - color: "#00A3E0" # Blue headings + color: "${colors.primary}" # Primary color for headings bold: true h1: - color: "#FFFFFF" # White text - background_color: "#9B51E0" # Purple background + color: "${colors.primary}" # Primary color + background_color: "${colors.background}" # Default background bold: true margin: 2 code_block: - color: "#00A3E0" # Blue code + color: "${colors.code}" # Code color margin: 1 link: - color: "#00A3E0" # Blue links + color: "${colors.primary}" # Primary color for links underline: trueLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
website/docs/cli/configuration/markdown-styling.mdx (1)
169-169: Fix minor typographical issues.A few small improvements to enhance readability:
- Line 169: Add "the" before "nearest"
- Line 183: Use an em dash instead of a hyphen
- Line 252: Add a period after "etc"
- - Automatically maps hex colors to nearest ANSI 256 colors + - Automatically maps hex colors to the nearest ANSI 256 colors -The color degradation is handled automatically by termenv's color profile detection. You don't need to configure anything - your styles will work everywhere +The color degradation is handled automatically by termenv's color profile detection. You don't need to configure anything — your styles will work everywhere - - Use a terminal emulator known for true color support (e.g., Terminal.app, iTerm2, Windows Terminal, etc). + - Use a terminal emulator known for true color support (e.g., Terminal.app, iTerm2, Windows Terminal, etc.).Also applies to: 183-183, 252-252
🧰 Tools
🪛 LanguageTool
[grammar] ~169-~169: A determiner may be missing.
Context: ...** - Automatically maps hex colors to nearest ANSI 256 colors - Detected via `$TERM...(THE_SUPERLATIVE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
atmos.yaml(1 hunks)website/docs/cli/configuration/markdown-styling.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/cli/configuration/markdown-styling.mdx
[grammar] ~169-~169: A determiner may be missing.
Context: ...** - Automatically maps hex colors to nearest ANSI 256 colors - Detected via `$TERM...
(THE_SUPERLATIVE)
[typographical] ~183-~183: To join two clauses or introduce examples, consider using an em dash.
Context: ...on. You don't need to configure anything - your styles will work everywhere, automa...
(DASH_RULE)
[style] ~252-~252: In American English, abbreviations like “etc.” require a period.
Context: ...Terminal.app, iTerm2, Windows Terminal, etc). 3. **Configure Environment Variables...
(ETC_PERIOD)
🔇 Additional comments (3)
website/docs/cli/configuration/markdown-styling.mdx (1)
55-132: Well-documented style properties!The documentation clearly explains all available properties and their usage for each markdown element.
atmos.yaml (2)
328-345: LGTM! Well-structured markdown styling configuration.The markdown styling configuration is well-organized and uses variables for colors, making it maintainable and consistent.
319-326: 🛠️ Refactor suggestionAdd theme configuration under terminal settings.
The terminal section needs a theme configuration to define the color variables used in markdown styling.
terminal: max_width: 120 # Maximum width for terminal output pager: true # Use pager for long output timestamps: false # Show timestamps in logs colors: true # Enable colored output unicode: true # Use unicode characters + theme: + colors: + text: "#F7FAFC" # Off white + primary: "#00A3E0" # Blue + secondary: "#9B51E0" # Purple + muted: "#A0AEC0" # Light gray + background: "#1A202C" # Dark slateLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
website/docs/cli/configuration/markdown-styling.mdx (3)
137-157: Update default styles to use theme variables.The default styles use hardcoded colors instead of referencing theme variables, which is inconsistent with the configuration example above.
Update the default styles to use theme variables:
settings: markdown: document: - color: "#FFFFFF" # White text + color: "${colors.text}" heading: - color: "#00A3E0" # Blue headings + color: "${colors.primary}" bold: true h1: - color: "#FFFFFF" # White text - background_color: "#9B51E0" # Purple background + color: "${colors.text}" + background_color: "${colors.secondary}" bold: true margin: 2 code_block: - color: "#00A3E0" # Blue code + color: "${colors.secondary}" margin: 1 link: - color: "#00A3E0" # Blue links + color: "${colors.primary}" underline: true
169-169: Fix minor grammatical and style issues.A few minor issues in the text:
- Line 169: Missing determiner before "nearest"
- Line 183: Consider using an em dash for better readability
- Line 252: Missing period after "etc" in American English
Apply these fixes:
-Automatically maps hex colors to nearest ANSI 256 colors +Automatically maps hex colors to the nearest ANSI 256 colors -You don't need to configure anything - your styles will work everywhere +You don't need to configure anything — your styles will work everywhere -Terminal.app, iTerm2, Windows Terminal, etc) +Terminal.app, iTerm2, Windows Terminal, etc.)Also applies to: 183-183, 252-252
🧰 Tools
🪛 LanguageTool
[grammar] ~169-~169: A determiner may be missing.
Context: ...** - Automatically maps hex colors to nearest ANSI 256 colors - Detected via `$TERM...(THE_SUPERLATIVE)
229-233: Enhance accessibility best practices.The best practices section could benefit from more specific guidance on color contrast ratios and tools for verification.
Add these specific recommendations:
1. **Color Contrast**: Ensure sufficient contrast between text and background colors for readability. + - Use tools like [WebAIM Contrast Checker](https://webaim.org/resources/contrastchecker/) + - Aim for a minimum contrast ratio of 4.5:1 for normal text 2. **Consistent Styling**: Use a consistent color scheme across different elements. 3. **Terminal Support**: Test your styling in different terminals to ensure compatibility. 4. **Accessibility**: Consider color-blind users when choosing your color scheme. + - Avoid problematic color combinations (red/green, blue/purple) + - Test with color blindness simulators
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/docs/cli/configuration/markdown-styling.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/cli/configuration/markdown-styling.mdx
[grammar] ~169-~169: A determiner may be missing.
Context: ...** - Automatically maps hex colors to nearest ANSI 256 colors - Detected via `$TERM...
(THE_SUPERLATIVE)
[typographical] ~183-~183: To join two clauses or introduce examples, consider using an em dash.
Context: ...on. You don't need to configure anything - your styles will work everywhere, automa...
(DASH_RULE)
[style] ~252-~252: In American English, abbreviations like “etc.” require a period.
Context: ...Terminal.app, iTerm2, Windows Terminal, etc). 3. **Configure Environment Variables...
(ETC_PERIOD)
🔇 Additional comments (2)
website/docs/cli/configuration/markdown-styling.mdx (2)
1-6: LGTM! Clear and well-structured frontmatter.The metadata is well-organized with appropriate title, labels, and description.
214-226: Add version compatibility information.The section about Glamour library should include version compatibility information since it's a critical dependency.
Let's verify the Glamour version used in the project:
Consider adding a note about the minimum supported version of Glamour and any known compatibility issues.
✅ Verification successful
Add Glamour v0.8.0 compatibility information
The documentation should mention that Atmos uses Glamour v0.8.0 for markdown rendering. Here's the suggested addition after the first paragraph:
Atmos uses the [Glamour](https://github.com/charmbracelet/glamour) library (v0.8.0) for markdown rendering and styling. The styling is handled automatically based on your terminal's capabilities and color profile.This version information helps users understand the exact capabilities and compatibility of the markdown rendering system they're working with.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Glamour version in go.mod rg -A 1 "github.com/charmbracelet/glamour" "go.mod"Length of output: 127
Script:
#!/bin/bash # Check for any version-specific code or compatibility checks rg -i "glamour.*version|version.*glamour" -g '!{*.md,*.mdx}' # Look for any Glamour-related configuration or initialization ast-grep --pattern 'glamour.New($$$)'Length of output: 102
Script:
#!/bin/bash # Check changelog or release notes for any version-related information fd "CHANGELOG|HISTORY" --type f --exec cat {}Length of output: 3893
Script:
#!/bin/bash # Check for any Glamour-related issues or discussions in documentation rg -i "glamour" -g '*.md' -g '*.mdx' -A 2 -B 2Length of output: 1555
|
These changes were released in v1.135.0. |
| ) | ||
|
|
||
| // ParseMarkdownSections parses a markdown string and returns a map of section titles to their content | ||
| func ParseMarkdownSections(content string) map[string]string { |
There was a problem hiding this comment.
- I think this was not required we can create
nnumber of markdown and that should be fine. - This function was used in workflow but i would say it does not work with the current version of workflow.md. I know this because i wrote a short test case because i could not believe the function worked for
workflow.md.
Implement Custom Markdown Styling for Workflow Commands
What
atmos.yamlWhy
Technical Details
markdownsettings section inatmos.yamlfor custom styling configurationReferences
Testing
The implementation has been tested with:
atmos.yamlEvidence:
Evidence:
https://github.com/charmbracelet/bubbletea
https://github.com/charmbracelet
https://github.com/charmbracelet/glow
https://github.com/charmbracelet/glamour/blob/master/styles/gallery/README.md
Summary by CodeRabbit
Release Notes
New Features
atmos workflowcommand usage.Documentation
Improvements
Bug Fixes