Beautify UI: improve spacing, padding, and visual hierarchy#27
Open
Beautify UI: improve spacing, padding, and visual hierarchy#27
Conversation
…l panels Add card-like section containers with rounded corners and subtle borders, a distinct sidebar background, tighter pane grid spacing, and consistent text sizing and button padding throughout the workspace builder, response panel, headers editor, auth editor, and status bar.
WalkthroughThis PR adds CI changes to post PR comments with links and metadata for e2e screenshots, adds the iced_fonts dependency, introduces a centralized icons module and Bootstrap icon wrappers, and implements a design-token-driven theme (spacing, radius, typo) with method/status color helpers and several container/button styles. Many UI modules were refactored to use the new theme tokens and icon set handling (including a new IconSet enum and icon helpers). It also handles late-arriving automation screenshots gracefully by making that path a no-op. Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Actions Runner
participant Job as e2e-automation job script
participant ArtifactStore as Actions Artifacts (artifacts/e2e)
participant GHCLI as gh CLI
participant GitHub as GitHub PR
Runner->>Job: start e2e-automation (on PR)
Job->>ArtifactStore: list/download PNGs in artifacts/e2e
alt screenshots found
Job->>Job: count files, gather names & sizes
Job->>Job: build Markdown body with links and sizes (uses run URL & artifact info)
Job->>GHCLI: gh pr comment --body "<markdown>"
GHCLI->>GitHub: POST comment to PR
GitHub-->>GHCLI: comment created
GHCLI-->>Job: success
else no screenshots
Job->>Job: skip posting (no-op)
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Automation Screenshots3 screenshot(s) captured during this CI run.
Download: automation-e2e-artifacts (see Artifacts section at bottom of the run page) |
Unify the app around Bootstrap icon fonts and reusable theme tokens so the interface feels consistent and polished across sidebar, workspace, auth, headers, and response views.
Automation Screenshots3 screenshot(s) captured during this CI run.
Download: automation-e2e-artifacts (see Artifacts section at bottom of the run page) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/app/view/workspace.rs (1)
65-65: Consider splittingbuilder_formto eliminate the#[allow(clippy::too_many_lines)]suppression.The
too_many_linesattribute is a workaround. Extracting the meta, request, and form-assembly logic into dedicated helper functions (e.g.,meta_row,request_row) would remove the need for the suppression and improve readability, consistent with the project's strict clippy baseline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/view/workspace.rs` at line 65, The function builder_form is currently large and uses #[allow(clippy::too_many_lines)]; split it by extracting logical sections into smaller helpers: create meta_row(&self, ctx: &Context) for the metadata rows, request_row(&self, ctx: &Context) for the request-related UI, and assemble_form(&self, ctx: &Context) (or similar) to compose the final form from those pieces; move the corresponding blocks of code and any local variables they need into these helpers (passing minimal arguments) and replace the inlined blocks in builder_form with calls to meta_row, request_row, and assemble_form so the suppression can be removed.src/icons.rs (1)
122-132: Consider removing unusedhourglassandlockfunctions (YAGNI).Both functions carry
#[allow(dead_code)], meaning nothing in the codebase currently calls them. Given the project enforceswarnings = "deny"and the YAGNI principle, dead icon wrappers should be deleted rather than suppressed. They can be re-added with a concrete use site when actually needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/icons.rs` around lines 122 - 132, Remove the unused icon wrapper functions hourglass and lock (and their #[allow(dead_code)] attributes) from src/icons.rs: delete the pub fn hourglass<'a>() -> Text<'a> and pub fn lock<'a>() -> Text<'a> wrappers (and their doc comments) since nothing calls them; reintroduce them later with a real use site if needed..github/workflows/ci.yml (1)
92-92: Unusedlistingvariable.
listingis populated at line 92 but the body-building loop at line 98 independently re-runsfind. The variable is never referenced and can be removed.🧹 Proposed cleanup
- listing=$(find artifacts/e2e -name '*.png' -printf '%f\n' 2>/dev/null | sort) - body="## Automation Screenshots"$'\n\n'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 92, Remove the unused variable assignment "listing=$(find artifacts/e2e -name '*.png' -printf '%f\n' 2>/dev/null | sort)" since the body-building loop re-runs find independently; delete that line or instead reuse the variable by replacing the second find in the body-building loop with iterating over "$listing" (ensure quoting), so either eliminate the redundant assignment or wire the loop to use the existing "listing" variable.src/app/view/sidebar.rs (1)
26-33: Normalize env input at parse boundary.
ZAGEL_ICON_SETparsing should trim whitespace before matching to avoid surprising fallbacks from values like" ascii ".♻️ Small hardening change
impl IconSet { pub fn from_env() -> Self { match std::env::var("ZAGEL_ICON_SET") .ok() - .map(|value| value.to_ascii_lowercase()) + .map(|value| value.trim().to_ascii_lowercase()) { Some(value) if value == "ascii" => Self::Ascii, _ => Self::Bootstrap, } } }As per coding guidelines, "Parse, don't validate: parse input at the edges so downstream state stays valid and doesn't need repeated validation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/view/sidebar.rs` around lines 26 - 33, The env parsing in Sidebar::from_env (reading ZAGEL_ICON_SET) should trim surrounding whitespace before normalizing to lowercase and matching so inputs like " ascii " don't fall through; update the map/closure that currently calls to_ascii_lowercase() to trim the input first (then lowercase) and use the resulting trimmed/lowercased value for the "ascii" equality check.src/app/view/response.rs (1)
267-300: Build tab content lazily to avoid unnecessary editor/highlighter work.
body_editorandbody_sectionare currently built even when the Headers tab is active. For large responses, this adds avoidable render cost.♻️ Suggested refactor (construct only the active tab)
- let pretty_kind = body.pretty_kind(); - let syntax = body.syntax(); - let body_editor = text_editor(content) - .height(Length::Fill) - .highlight(syntax.as_str(), highlight_theme) - .wrapping(Wrapping::None); - - let body_section: Element<'_, Message> = column![ - text(format!( - "Body ({})", - match (display, pretty_kind) { - (ResponseDisplay::Pretty, Some(PrettyKind::Html)) => { - "pretty (HTML; formatted view)" - } - (ResponseDisplay::Pretty, Some(PrettyKind::Json)) => "pretty (JSON)", - (ResponseDisplay::Pretty, None) => "pretty (raw shown)", - (ResponseDisplay::Raw, _) => "raw", - } - )) - .size(typo::CAPTION), - body_editor, - ] - .spacing(spacing::XS) - .into(); - - let headers_section: Element<'_, Message> = - column![text("Headers").size(typo::CAPTION), headers_view,] - .spacing(spacing::XS) - .into(); - - let tab_view: Element<'_, Message> = match tab { - ResponseTab::Body => body_section, - ResponseTab::Headers => headers_section, - }; + let tab_view: Element<'_, Message> = match tab { + ResponseTab::Body => { + let pretty_kind = body.pretty_kind(); + let syntax = body.syntax(); + let body_editor = text_editor(content) + .height(Length::Fill) + .highlight(syntax.as_str(), highlight_theme) + .wrapping(Wrapping::None); + + column![ + text(format!( + "Body ({})", + match (display, pretty_kind) { + (ResponseDisplay::Pretty, Some(PrettyKind::Html)) => { + "pretty (HTML; formatted view)" + } + (ResponseDisplay::Pretty, Some(PrettyKind::Json)) => "pretty (JSON)", + (ResponseDisplay::Pretty, None) => "pretty (raw shown)", + (ResponseDisplay::Raw, _) => "raw", + } + )) + .size(typo::CAPTION), + body_editor, + ] + .spacing(spacing::XS) + .into() + } + ResponseTab::Headers => column![ + text("Headers").size(typo::CAPTION), + headers_view + ] + .spacing(spacing::XS) + .into(), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/view/response.rs` around lines 267 - 300, The body editor and its enclosing UI (variables/methods: pretty_kind, syntax, body_editor, body_section, text_editor) are always constructed even when ResponseTab::Headers is selected; change the match that builds tab_view so that headers_section is used directly for ResponseTab::Headers and the body-related values (compute pretty_kind, syntax and call text_editor to produce body_editor and body_section) are only computed inside the ResponseTab::Body arm; keep headers_section and its construction (headers_view) unchanged and ensure ResponseDisplay logic that formats the "Body (...)" label is evaluated only when building the Body branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 98-102: The for-loop using command substitution with $(find ...)
will break on filenames with spaces; change it to a null-delimited iteration
using find -print0 piped into a read loop (e.g., use find ... -print0 | while
IFS= read -r -d '' img; do ...) so each filename (img) is read safely; inside
the loop keep using basename to set name and du -h "$img" | cut -f1 to set size
and append to body exactly as before, ensuring all uses of img are quoted to
preserve spaces.
In `@src/app/view/sidebar.rs`:
- Around line 87-97: The action buttons (e.g., the add_project button using
icons::plus_circle) hardcode Bootstrap glyphs and ignore IconSet::Ascii; replace
the icon construction with the same match on ctx.icon_set used for tree rows so
ASCII mode uses the ASCII fallback (i.e., when ctx.icon_set == IconSet::Ascii
choose the ascii glyph string/icon, otherwise use
icons::plus_circle().size(...)), and apply this pattern to every action-row icon
occurrence (dash_circle, trash, check_lg, pencil) so all controls honor
IconSet::Ascii and use the documented fallback.
In `@src/theme.rs`:
- Around line 12-22: The module-level doc for spacing is incorrect: it lists
only "4 / 8 / 12 / 16 / 24 / 32" but the spacing module actually defines XXXS,
XXS, XS, SM, MD, LG, XL, XXL (values 2.0, 4.0, 6.0, 8.0, 12.0, 16.0, 24.0,
32.0). Update the comment above pub mod spacing to accurately list the token
scale (e.g. "Spacing tokens (px): 2 / 4 / 6 / 8 / 12 / 16 / 24 / 32") so it
matches the constants XXXS, XXS, XS, SM, MD, LG, XL, and XXL.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 92: Remove the unused variable assignment "listing=$(find artifacts/e2e
-name '*.png' -printf '%f\n' 2>/dev/null | sort)" since the body-building loop
re-runs find independently; delete that line or instead reuse the variable by
replacing the second find in the body-building loop with iterating over
"$listing" (ensure quoting), so either eliminate the redundant assignment or
wire the loop to use the existing "listing" variable.
In `@src/app/view/response.rs`:
- Around line 267-300: The body editor and its enclosing UI (variables/methods:
pretty_kind, syntax, body_editor, body_section, text_editor) are always
constructed even when ResponseTab::Headers is selected; change the match that
builds tab_view so that headers_section is used directly for
ResponseTab::Headers and the body-related values (compute pretty_kind, syntax
and call text_editor to produce body_editor and body_section) are only computed
inside the ResponseTab::Body arm; keep headers_section and its construction
(headers_view) unchanged and ensure ResponseDisplay logic that formats the "Body
(...)" label is evaluated only when building the Body branch.
In `@src/app/view/sidebar.rs`:
- Around line 26-33: The env parsing in Sidebar::from_env (reading
ZAGEL_ICON_SET) should trim surrounding whitespace before normalizing to
lowercase and matching so inputs like " ascii " don't fall through; update the
map/closure that currently calls to_ascii_lowercase() to trim the input first
(then lowercase) and use the resulting trimmed/lowercased value for the "ascii"
equality check.
In `@src/app/view/workspace.rs`:
- Line 65: The function builder_form is currently large and uses
#[allow(clippy::too_many_lines)]; split it by extracting logical sections into
smaller helpers: create meta_row(&self, ctx: &Context) for the metadata rows,
request_row(&self, ctx: &Context) for the request-related UI, and
assemble_form(&self, ctx: &Context) (or similar) to compose the final form from
those pieces; move the corresponding blocks of code and any local variables they
need into these helpers (passing minimal arguments) and replace the inlined
blocks in builder_form with calls to meta_row, request_row, and assemble_form so
the suppression can be removed.
In `@src/icons.rs`:
- Around line 122-132: Remove the unused icon wrapper functions hourglass and
lock (and their #[allow(dead_code)] attributes) from src/icons.rs: delete the
pub fn hourglass<'a>() -> Text<'a> and pub fn lock<'a>() -> Text<'a> wrappers
(and their doc comments) since nothing calls them; reintroduce them later with a
real use site if needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci.ymlCargo.tomlsrc/app/headers.rssrc/app/lifecycle.rssrc/app/view/auth.rssrc/app/view/mod.rssrc/app/view/response.rssrc/app/view/sidebar.rssrc/app/view/workspace.rssrc/icons.rssrc/main.rssrc/theme.rs
Ignore late screenshot callbacks after automation completion, make sidebar action icons honor ASCII fallback, harden CI screenshot file iteration for spaces, and sync spacing token docs with actual values.
Automation Screenshots3 screenshot(s) captured during this CI run.
Download: automation-e2e-artifacts (see Artifacts section at bottom of the run page) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/view/sidebar.rs (1)
519-531: Deduplicate up/down move icon rendering.The same
match ctx.icon_setblocks for up/down controls are repeated in both collection and request rows. Extractingmove_up_icon/move_down_iconhelpers would keep behavior consistent and reduce churn.Also applies to: 609-621
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/view/sidebar.rs` around lines 519 - 531, Extract the repeated icon-selection logic into two small helper functions (e.g., move_up_icon(ctx: &Context) -> Element and move_down_icon(ctx: &Context) -> Element) and replace the duplicated match blocks that call ctx.icon_set in the collection and request row code with calls to these helpers; each helper should match IconSet::Bootstrap to return icons::arrow_up()/icons::arrow_down().size(typo::CAPTION).into() and IconSet::Ascii to return Element::from(text("^"/"v").size(typo::CAPTION)), preserving existing sizing and types so the .padding/.style/.on_press chains remain unchanged (apply same change for the other duplicated block noted around lines 609-621).src/theme.rs (1)
174-277: Extract shared filled-button state logic to avoid style drift.
accent_button_styleanddestructive_button_styleduplicate the same status-state shading flow. A shared helper would reduce maintenance risk.♻️ Refactor sketch
+fn shift_rgb(c: Color, delta: f32) -> Color { + Color::from_rgba( + (c.r + delta).clamp(0.0, 1.0), + (c.g + delta).clamp(0.0, 1.0), + (c.b + delta).clamp(0.0, 1.0), + 1.0, + ) +} + +fn filled_button_style(base: Color, text: Color, status: button::Status) -> button::Style { + let (bg, text_alpha) = match status { + button::Status::Active => (base, 1.0), + button::Status::Hovered => (shift_rgb(base, 0.08), 1.0), + button::Status::Pressed => (shift_rgb(base, -0.06), 1.0), + button::Status::Disabled => (Color::from_rgba(base.r, base.g, base.b, 0.4), 0.5), + }; + + button::Style { + background: Some(bg.into()), + text_color: Color::from_rgba(text.r, text.g, text.b, text_alpha), + border: border::rounded(radius::SM), + ..button::Style::default() + } +} + pub fn accent_button_style(theme: &Theme, status: button::Status) -> button::Style { let palette = theme.extended_palette(); - let accent = palette.primary.strong; - match status { ... } + filled_button_style(palette.primary.strong.color, palette.primary.strong.text, status) } pub fn destructive_button_style(theme: &Theme, status: button::Status) -> button::Style { let palette = theme.extended_palette(); - let danger = palette.danger.strong; - match status { ... } + filled_button_style(palette.danger.strong.color, palette.danger.strong.text, status) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/theme.rs` around lines 174 - 277, Both accent_button_style and destructive_button_style duplicate the same status-based shading/opacity logic; extract that into a shared helper (e.g. filled_button_style or style_for_filled_button) that accepts the base Color and text Color (or a struct like PaletteColor { color, text }) and the button::Status, then return the button::Style using the same adjustments for Hovered (+0.08 clamped), Pressed (-0.06 clamped), Active (base color), and Disabled (alpha 0.4 for background, 0.5 for text) and reuse border::rounded(radius::SM) and ..button::Style::default(); update accent_button_style and destructive_button_style to call this helper with palette.primary.strong and palette.danger.strong respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/view/sidebar.rs`:
- Around line 287-301: The three section headers (project_section,
global_env_section, collections_section) currently pass hardcoded Bootstrap
glyphs (icons::folder_open(), icons::globe(), icons::collection()) which bypass
the IconSet::Ascii fallback; update these calls to use the project's icon
resolution helper that respects IconSet::Ascii (the same icon helper used
elsewhere in the codebase) instead of the direct icons::* constructors so the
section(...) invocations use the fallback ASCII glyphs when IconSet::Ascii is
selected.
---
Nitpick comments:
In `@src/app/view/sidebar.rs`:
- Around line 519-531: Extract the repeated icon-selection logic into two small
helper functions (e.g., move_up_icon(ctx: &Context) -> Element and
move_down_icon(ctx: &Context) -> Element) and replace the duplicated match
blocks that call ctx.icon_set in the collection and request row code with calls
to these helpers; each helper should match IconSet::Bootstrap to return
icons::arrow_up()/icons::arrow_down().size(typo::CAPTION).into() and
IconSet::Ascii to return Element::from(text("^"/"v").size(typo::CAPTION)),
preserving existing sizing and types so the .padding/.style/.on_press chains
remain unchanged (apply same change for the other duplicated block noted around
lines 609-621).
In `@src/theme.rs`:
- Around line 174-277: Both accent_button_style and destructive_button_style
duplicate the same status-based shading/opacity logic; extract that into a
shared helper (e.g. filled_button_style or style_for_filled_button) that accepts
the base Color and text Color (or a struct like PaletteColor { color, text })
and the button::Status, then return the button::Style using the same adjustments
for Hovered (+0.08 clamped), Pressed (-0.06 clamped), Active (base color), and
Disabled (alpha 0.4 for background, 0.5 for text) and reuse
border::rounded(radius::SM) and ..button::Style::default(); update
accent_button_style and destructive_button_style to call this helper with
palette.primary.strong and palette.danger.strong respectively.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.ymlsrc/app/automation.rssrc/app/view/sidebar.rssrc/theme.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
Comment on lines
+287
to
+301
| let project_section = section( | ||
| "Projects", | ||
| icons::folder_open().size(typo::BODY), | ||
| project_roots.into(), | ||
| ); | ||
| let global_env_section = section( | ||
| "Global Environments", | ||
| icons::globe().size(typo::BODY), | ||
| global_env_roots.into(), | ||
| ); | ||
| let collections_section = section( | ||
| "Collections", | ||
| icons::collection().size(typo::BODY), | ||
| list.into(), | ||
| ); |
There was a problem hiding this comment.
Section header icons still bypass IconSet::Ascii fallback.
These three section icons are hardcoded Bootstrap glyphs, so ASCII mode is still incomplete and can show missing glyphs in fallback environments.
🛠️ Proposed fix
- let project_section = section(
- "Projects",
- icons::folder_open().size(typo::BODY),
- project_roots.into(),
- );
+ let project_icon = match ctx.icon_set {
+ IconSet::Bootstrap => icons::folder_open().size(typo::BODY).into(),
+ IconSet::Ascii => text("[P]").size(typo::CAPTION).into(),
+ };
+ let project_section = section("Projects", project_icon, project_roots.into());
- let global_env_section = section(
- "Global Environments",
- icons::globe().size(typo::BODY),
- global_env_roots.into(),
- );
+ let global_icon = match ctx.icon_set {
+ IconSet::Bootstrap => icons::globe().size(typo::BODY).into(),
+ IconSet::Ascii => text("[G]").size(typo::CAPTION).into(),
+ };
+ let global_env_section = section("Global Environments", global_icon, global_env_roots.into());
- let collections_section = section(
- "Collections",
- icons::collection().size(typo::BODY),
- list.into(),
- );
+ let collections_icon = match ctx.icon_set {
+ IconSet::Bootstrap => icons::collection().size(typo::BODY).into(),
+ IconSet::Ascii => text("[C]").size(typo::CAPTION).into(),
+ };
+ let collections_section = section("Collections", collections_icon, list.into());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/view/sidebar.rs` around lines 287 - 301, The three section headers
(project_section, global_env_section, collections_section) currently pass
hardcoded Bootstrap glyphs (icons::folder_open(), icons::globe(),
icons::collection()) which bypass the IconSet::Ascii fallback; update these
calls to use the project's icon resolution helper that respects IconSet::Ascii
(the same icon helper used elsewhere in the codebase) instead of the direct
icons::* constructors so the section(...) invocations use the fallback ASCII
glyphs when IconSet::Ascii is selected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
theme.rsChanges
src/theme.rssection_container_style,status_bar_style,sidebar_container_stylesrc/app/view/mod.rssection()helper to use card-style containers, smaller section titles, tighter pane spacing, styled status barsrc/app/view/sidebar.rssrc/app/view/workspace.rssrc/app/view/response.rssrc/app/view/auth.rssrc/app/headers.rsAll changes pass
cargo clippy --all-targets --all-features -- -D warnings.Summary by CodeRabbit
New Features
Improvements
Bug Fixes