feat(ui): refine tool cards and change review - #131
Conversation
Greptile SummaryThe PR streamlines tool-card styling and workspace metadata presentation while improving patch/review operation classification, rename display, scrolling, and initial expansion behavior.
Confidence Score: 4/5The missing workspace skill-diagnostics rendering should be fixed before merging because affected users lose the only visible explanation for skill-loading problems. open_workspace continues to emit skill diagnostics and treats them as expandable content, but the new structured payload renderer omits them and can display an empty-details message instead. Files Needing Attention: src/ui/workspace-app.tsx
|
| Filename | Overview |
|---|---|
| src/ui/workspace-app.tsx | Reworks card expansion and workspace payload rendering, but drops the still-produced skill diagnostics from the expanded UI. |
| src/ui/patch-display.ts | Centralizes file operation classification, rename-path formatting, and rendered diff/card correlation. |
| src/ui/review-payload.tsx | Adds immediate single-file rendering and richer multi-file operation and rename headers. |
| src/apply-patch.ts | Correctly reports an Add File directive that overwrites an existing file as an update. |
| src/ui/card-types.ts | Adds typed review operations and initial-expansion policy for review and single-file patch cards. |
| src/ui/workspace-app.css | Refines card sizing, semantic accents, workspace disclosures, review rows, and responsive styling. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Tool result card] --> B{Tool type}
B -->|apply_patch / show_changes| C[Parse diff files]
C --> D[Classify operation and paths]
D --> E[Render single diff or expandable file rows]
B -->|open_workspace| F[Render structured metadata rows]
F --> G[Instructions and previews]
F --> H[Skills, providers, and agents]
Reviews (1): Last reviewed commit: "feat(ui): open single-file patch diffs" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const agentNames = (card.agents ?? []).map((agent) => { | ||
| const name = agent.name ?? "Unnamed agent"; | ||
| return agent.provider ? `${name} · ${agent.provider}` : name; | ||
| }); | ||
| if (agentNames.length > 0) { | ||
| appendWorkspaceTextListRow( | ||
| rows, | ||
| "Agents", | ||
| agentNames, | ||
| toolIcons.agents, | ||
| "agents", | ||
| ); | ||
| } |
There was a problem hiding this comment.
When open_workspace returns skill diagnostics, this renderer ignores them even though they make the card expandable, causing skill-loading errors to disappear and diagnostics-only cards to report “No workspace details available.”
There was a problem hiding this comment.
no need to show it to user, it's pretty much internal details
There was a problem hiding this comment.
Got it — keeping diagnostics as internal-only makes sense. I'll drop that concern.
Shall I remember this as a rule: "skill diagnostics from open_workspace are internal details and should not be surfaced in the UI"?
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change aligns patch result classification with file-change display, adds initial card expansion rules, replaces plain-text workspace payloads with structured rendering, and updates icons, scrollbars, accessibility, responsive layouts, and card styling. ChangesPatch and workspace UI
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ToolDisplay
participant WorkspaceApp
participant DOM
ToolDisplay->>WorkspaceApp: identify open_workspace payload
WorkspaceApp->>DOM: render structured workspace details
WorkspaceApp->>DOM: preserve or reset disclosure and preview state
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/ui/tool-display.ts (1)
112-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the duplicated ternary branch.
Both the
fileCount > 0branch and thecard.payload?.patchbranch returndisplay.title. The nested conditional therefore reduces to a single boolean test.♻️ Proposed simplification
- title: fileCount > 0 - ? display.title - : card.payload?.patch - ? display.title - : "No changes", + title: fileCount > 0 || card.payload?.patch + ? display.title + : "No changes",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/tool-display.ts` around lines 112 - 116, In the title expression within the tool display construction, collapse the nested ternary so display.title is selected when either fileCount > 0 or card.payload?.patch is truthy; otherwise return "No changes".src/ui/workspace-app.tsx (2)
532-544: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface agent availability and model data instead of dropping it.
agentNameskeeps onlynameandprovider. The card also carriesmodel,thinking,providerAvailable, andproviderUnavailableReason(seesrc/ui/card-types.tsLines 70-79). The rendered workspace details discard all four.This is inconsistent inside the same renderer. For providers at Lines 518-530 you preserve
reasonas the chip title and mark unavailable entries with themutedtone. An agent whose provider is unavailable renders identically to a usable agent, so the user cannot tell which agents they can actually run.Consider rendering agents as chips with the same tone and title treatment used for providers.
♻️ Proposed change to preserve agent availability
- const agentNames = (card.agents ?? []).map((agent) => { - const name = agent.name ?? "Unnamed agent"; - return agent.provider ? `${name} · ${agent.provider}` : name; - }); - if (agentNames.length > 0) { - appendWorkspaceTextListRow( - rows, - "Agents", - agentNames, - toolIcons.agents, - "agents", - ); - } + const agents = card.agents ?? []; + if (agents.length > 0) { + const agentChips: WorkspaceChip[] = agents.map((agent) => { + const name = agent.name ?? "Unnamed agent"; + const unavailable = agent.providerAvailable === false; + const details = [agent.provider, agent.model].filter(Boolean).join(" · "); + return { + label: details ? `${name} · ${details}` : name, + tone: unavailable ? "muted" : undefined, + title: unavailable + ? agent.providerUnavailableReason ?? "Provider unavailable" + : undefined, + }; + }); + appendWorkspaceChipRow(rows, "Agents", agentChips, toolIcons.agents); + }Based on learnings from the coding guidelines: "Preserve host and provider data unless DevSpace has a concrete reason to normalize it, and add compatibility behavior only for an identified consumer with a real upgrade path."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/workspace-app.tsx` around lines 532 - 544, Update the agent rendering in the workspace details renderer to preserve each agent’s model, thinking, providerAvailable, and providerUnavailableReason data instead of reducing agents to plain name strings. Render agents as chips using the same availability tone and reason title behavior as the provider rendering near the existing provider logic, while retaining the current name/provider display and Agents row behavior.Source: Coding guidelines
862-902: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared disclosure row builder.
appendWorkspaceSkills(Lines 875-901),appendWorkspaceTextListRow(Lines 802-835), andappendWorkspaceInstructions(Lines 595-632) each build the same disclosure scaffolding: theworkspace-row workspace-row-disclosureclass with a conditionalexpandedsuffix, the matchingworkspace-disclosurespan, a toggle that flips both classes, and the same add/delete calls againstexpandedWorkspaceDisclosures.Three copies of this logic will drift. Extract one helper that accepts the label, the icon, the disclosure key, the content element, the item total, and an optional extra class name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/workspace-app.tsx` around lines 862 - 902, Extract the duplicated disclosure-row construction from appendWorkspaceSkills, appendWorkspaceTextListRow, and appendWorkspaceInstructions into one shared helper. Have it accept the label, icon, disclosure key, content element, item total, and optional extra class name, while preserving the existing expanded-state class toggling and expandedWorkspaceDisclosures add/delete behavior. Replace each local scaffold with calls to the helper, retaining each row’s existing content and styling.src/ui/heavy-payload.tsx (1)
152-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one diff options builder with
review-payload.tsx.This options object now matches
diffOptionsinsrc/ui/review-payload.tsxLines 196-212 on eleven fields, including the four this PR added here:unsafeCSS,collapsedContextThreshold,expansionLineCount, anddisableFileHeader. The only difference isstickyHeader, which istruehere andfalsethere while both setdisableFileHeader: true.Extract a shared builder that takes
themeTypeand thestickyHeadervalue. The two diff surfaces then cannot drift apart.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/heavy-payload.tsx` around lines 152 - 169, Extract the duplicated diff options object into a shared builder accepting themeType and stickyHeader, preserving all existing option values. Update the diff configuration in heavy-payload.tsx and review-payload.tsx to use this builder with their respective stickyHeader values, so both surfaces share one source of truth.src/ui/scrollbar.ts (1)
10-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the standard scrollbar properties for non-WebKit browsers.
The whole block sits inside
@supports selector(::-webkit-scrollbar). Firefox does not support that pseudo-element, so it evaluates to false and Firefox users get the default scrollbar. The standardscrollbar-widthandscrollbar-colorproperties cover Firefox and can sit outside the feature query.🎨 Proposed addition
[data-code] { scrollbar-gutter: auto; + scrollbar-width: thin; + scrollbar-color: var(--scrollbar-thumb, rgb(128 128 128 / 55%)) transparent; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/scrollbar.ts` around lines 10 - 41, Update the scrollbar styling around the [data-code] rules to add standard scrollbar-width and scrollbar-color declarations outside the WebKit feature query, using the existing thumb and transparent track colors. Keep the existing `@supports` selector(::-webkit-scrollbar) block unchanged for WebKit-specific styling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/patch-display.ts`:
- Around line 145-149: Update the matched-file selection near matchedFile and
cardFile to prefer indexedFile when its path matches parsedFile.path, using the
existing files.find path match only as a fallback. Preserve the current
previousPath matching behavior for fallback lookup, and add a test covering two
moves with the same destination path to verify the indexed row is displayed.
In `@src/ui/review-payload.tsx`:
- Around line 118-143: Update the renamed-file header in the review payload
rendering to expose its path text: add an appropriate naming role to the
file-kind badge span, and remove the aria-hidden attributes from the previous,
arrow, and current path spans so the renamed path remains readable to assistive
technology. Use the existing role="img" pattern from
renderWorkspaceInstructionStatus, while preserving the visual structure and
labels.
In `@src/ui/workspace-app.css`:
- Around line 178-207: Replace the shared --font-text-sm-size usage across the
affected typography roles with distinct sizing tokens or fixed values: preserve
the 14px tool-title size, use the appropriate smaller token for .tool-label and
.stats, and separately maintain the 13px, 11px, and 10px sizes used by
.review-diff-file-name, .workspace-chip, .workspace-instruction-preview, and
.workspace-instruction-path. Ensure host overrides cannot collapse these roles
into one size.
- Around line 615-616: Replace the deprecated word-break: break-word declaration
in the affected style block with overflow-wrap: break-word while preserving
white-space: pre-wrap. Also update the matching deprecated declaration in
.text-payload for consistency.
- Around line 11-12: Update the --tool-accent-soft definition in the tone
override rules so it is recomputed from each tone’s --tool-accent value, rather
than remaining fixed from :root. Preserve the existing color-mix behavior and
ensure .tool-icon.color receives the corresponding semantic tint for every tone.
In `@src/ui/workspace-app.tsx`:
- Around line 574-588: Update the loaded-file handling in the workspace
instruction rendering flow to preserve an undefined path instead of defaulting
to “AGENTS.md”; use a neutral display label for pathless files without
presenting it as a host-reported path. Change preview identity and
synchronization in the relevant instruction rendering and
syncWorkspaceInstructionPreviews logic to use each file’s index, including
dataset.instructionPath and matching, so multiple pathless files remain
distinct.
---
Nitpick comments:
In `@src/ui/heavy-payload.tsx`:
- Around line 152-169: Extract the duplicated diff options object into a shared
builder accepting themeType and stickyHeader, preserving all existing option
values. Update the diff configuration in heavy-payload.tsx and
review-payload.tsx to use this builder with their respective stickyHeader
values, so both surfaces share one source of truth.
In `@src/ui/scrollbar.ts`:
- Around line 10-41: Update the scrollbar styling around the [data-code] rules
to add standard scrollbar-width and scrollbar-color declarations outside the
WebKit feature query, using the existing thumb and transparent track colors.
Keep the existing `@supports` selector(::-webkit-scrollbar) block unchanged for
WebKit-specific styling.
In `@src/ui/tool-display.ts`:
- Around line 112-116: In the title expression within the tool display
construction, collapse the nested ternary so display.title is selected when
either fileCount > 0 or card.payload?.patch is truthy; otherwise return "No
changes".
In `@src/ui/workspace-app.tsx`:
- Around line 532-544: Update the agent rendering in the workspace details
renderer to preserve each agent’s model, thinking, providerAvailable, and
providerUnavailableReason data instead of reducing agents to plain name strings.
Render agents as chips using the same availability tone and reason title
behavior as the provider rendering near the existing provider logic, while
retaining the current name/provider display and Agents row behavior.
- Around line 862-902: Extract the duplicated disclosure-row construction from
appendWorkspaceSkills, appendWorkspaceTextListRow, and
appendWorkspaceInstructions into one shared helper. Have it accept the label,
icon, disclosure key, content element, item total, and optional extra class
name, while preserving the existing expanded-state class toggling and
expandedWorkspaceDisclosures add/delete behavior. Replace each local scaffold
with calls to the helper, retaining each row’s existing content and styling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8cdbd34e-eaaa-4f58-a252-86754724cb48
📒 Files selected for processing (14)
src/apply-patch.test.tssrc/apply-patch.tssrc/ui/card-types.test.tssrc/ui/card-types.tssrc/ui/heavy-payload.tsxsrc/ui/icons.tssrc/ui/patch-display.test.tssrc/ui/patch-display.tssrc/ui/review-payload.tsxsrc/ui/scrollbar.tssrc/ui/tool-display.test.tssrc/ui/tool-display.tssrc/ui/workspace-app.csssrc/ui/workspace-app.tsx
| const matchedFile = files.find((file) => ( | ||
| file.path === parsedFile.path && | ||
| (!parsedFile.previousPath || !file.previousPath || file.previousPath === parsedFile.previousPath) | ||
| )); | ||
| const cardFile = matchedFile ?? indexedFile; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prefer the indexed file before a path-only match.
files.find(...) returns the first entry for a repeated destination path. For apply_patch move diffs without parsed previousPath, a later row can display the source path from the first move. Use indexedFile when its path matches parsedFile.path, then use the path match as a fallback. Add a test with two moves to the same destination path.
Proposed fix
const indexedFile = files[index];
-const matchedFile = files.find((file) => (
+const matchedFile = indexedFile?.path === parsedFile.path
+ ? indexedFile
+ : files.find((file) => (
file.path === parsedFile.path &&
(!parsedFile.previousPath || !file.previousPath || file.previousPath === parsedFile.previousPath)
-));
+ ));
const cardFile = matchedFile ?? indexedFile;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const matchedFile = files.find((file) => ( | |
| file.path === parsedFile.path && | |
| (!parsedFile.previousPath || !file.previousPath || file.previousPath === parsedFile.previousPath) | |
| )); | |
| const cardFile = matchedFile ?? indexedFile; | |
| const matchedFile = indexedFile?.path === parsedFile.path | |
| ? indexedFile | |
| : files.find((file) => ( | |
| file.path === parsedFile.path && | |
| (!parsedFile.previousPath || !file.previousPath || file.previousPath === parsedFile.previousPath) | |
| )); | |
| const cardFile = matchedFile ?? indexedFile; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/patch-display.ts` around lines 145 - 149, Update the matched-file
selection near matchedFile and cardFile to prefer indexedFile when its path
matches parsedFile.path, using the existing files.find path match only as a
fallback. Preserve the current previousPath matching behavior for fallback
lookup, and add a test covering two moves with the same destination path to
verify the indexed row is displayed.
| <span | ||
| className={`review-file-kind ${changeKind}`} | ||
| title={fileChangeKindLabel(changeKind)} | ||
| aria-label={fileChangeKindLabel(changeKind)} | ||
| > | ||
| {fileChangeSymbol(changeKind)} | ||
| </span> | ||
| {pathDisplay?.previous ? ( | ||
| <span | ||
| className="review-diff-file-name renamed" | ||
| title={pathDisplay.title} | ||
| aria-label={pathDisplay.title} | ||
| > | ||
| <span className="review-diff-file-path previous" aria-hidden="true"> | ||
| {pathDisplay.previous} | ||
| </span> | ||
| <span className="review-diff-file-arrow" aria-hidden="true">→</span> | ||
| <span className="review-diff-file-path current" aria-hidden="true"> | ||
| {pathDisplay.current} | ||
| </span> | ||
| </span> | ||
| ) : ( | ||
| <span className="review-diff-file-name" title={pathDisplay?.title ?? fileDiff.name}> | ||
| {pathDisplay?.current ?? fileDiff.name} | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The renamed-file header has no accessible name for the path.
In the renamed branch, all three path spans use aria-hidden="true". The only remaining carrier of the path text is aria-label on the wrapping <span> at Line 126. A <span> has an implicit generic role, and ARIA does not permit naming generic elements, so browsers and screen readers commonly drop that aria-label. The expand button then announces only the change-kind symbol and the stats, without the file path.
The kind badge at Lines 118-124 has the same shape. This PR already solves it correctly in src/ui/workspace-app.tsx at Lines 745-750, where renderWorkspaceInstructionStatus sets role="img" next to aria-label.
Apply the same pattern here: give the badge role="img", and let the current path remain readable instead of hiding it.
♿ Proposed fix for the accessible name
<span
className={`review-file-kind ${changeKind}`}
+ role="img"
title={fileChangeKindLabel(changeKind)}
aria-label={fileChangeKindLabel(changeKind)}
>
{fileChangeSymbol(changeKind)}
</span>
{pathDisplay?.previous ? (
<span
className="review-diff-file-name renamed"
title={pathDisplay.title}
- aria-label={pathDisplay.title}
>
<span className="review-diff-file-path previous" aria-hidden="true">
{pathDisplay.previous}
</span>
<span className="review-diff-file-arrow" aria-hidden="true">→</span>
- <span className="review-diff-file-path current" aria-hidden="true">
+ <span className="review-diff-file-path current">
{pathDisplay.current}
</span>
</span>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <span | |
| className={`review-file-kind ${changeKind}`} | |
| title={fileChangeKindLabel(changeKind)} | |
| aria-label={fileChangeKindLabel(changeKind)} | |
| > | |
| {fileChangeSymbol(changeKind)} | |
| </span> | |
| {pathDisplay?.previous ? ( | |
| <span | |
| className="review-diff-file-name renamed" | |
| title={pathDisplay.title} | |
| aria-label={pathDisplay.title} | |
| > | |
| <span className="review-diff-file-path previous" aria-hidden="true"> | |
| {pathDisplay.previous} | |
| </span> | |
| <span className="review-diff-file-arrow" aria-hidden="true">→</span> | |
| <span className="review-diff-file-path current" aria-hidden="true"> | |
| {pathDisplay.current} | |
| </span> | |
| </span> | |
| ) : ( | |
| <span className="review-diff-file-name" title={pathDisplay?.title ?? fileDiff.name}> | |
| {pathDisplay?.current ?? fileDiff.name} | |
| </span> | |
| )} | |
| <span | |
| className={`review-file-kind ${changeKind}`} | |
| role="img" | |
| title={fileChangeKindLabel(changeKind)} | |
| aria-label={fileChangeKindLabel(changeKind)} | |
| > | |
| {fileChangeSymbol(changeKind)} | |
| </span> | |
| {pathDisplay?.previous ? ( | |
| <span | |
| className="review-diff-file-name renamed" | |
| title={pathDisplay.title} | |
| > | |
| <span className="review-diff-file-path previous" aria-hidden="true"> | |
| {pathDisplay.previous} | |
| </span> | |
| <span className="review-diff-file-arrow" aria-hidden="true">→</span> | |
| <span className="review-diff-file-path current"> | |
| {pathDisplay.current} | |
| </span> | |
| </span> | |
| ) : ( | |
| <span className="review-diff-file-name" title={pathDisplay?.title ?? fileDiff.name}> | |
| {pathDisplay?.current ?? fileDiff.name} | |
| </span> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/review-payload.tsx` around lines 118 - 143, Update the renamed-file
header in the review payload rendering to expose its path text: add an
appropriate naming role to the file-kind badge span, and remove the aria-hidden
attributes from the previous, arrow, and current path spans so the renamed path
remains readable to assistive technology. Use the existing role="img" pattern
from renderWorkspaceInstructionStatus, while preserving the visual structure and
labels.
| --tool-accent: var(--color-text-secondary, #b6b6bd); | ||
| --tool-accent-soft: color-mix(in srgb, var(--tool-accent) 12%, transparent); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
CSS custom property containing var() substituted at computed-value time on declaring element, not redefined by descendant
💡 Result:
CSS custom properties and var functions are resolved at computed-value time [1][2]. The substitution process occurs locally on the element where the property is being applied, using the computed value of the custom property as it exists on that same element [2]. When a var function is encountered, the browser looks for the custom property definition on the current element [2]. If the property is found, its computed value is substituted into the declaration [2]. If the property is not found on that element, the browser continues to look up the inheritance chain (if the custom property is inherited) to find the nearest ancestor that has the property defined [2]. Crucially, there is no "late binding" or reactive re-evaluation of variables across the DOM tree [3]. If you have a property that uses a var reference, that reference resolves to a static value once the computation for that specific element is complete [3][4]. If a descendant element redefines the custom property, it only affects declarations on that descendant (or its own descendants); it does not retroactively change the computed values of properties already calculated for its ancestors or siblings [3][5]. This behavior is fundamental to CSS computation: 1. Substitution happens once per element during the computed-value stage [4]. 2. Properties inherit their computed values, not their specified values (i.e., not the var expression itself) [3][4]. 3. Because inheritance occurs after the variable has been substituted and resolved into a final value, descendant elements cannot "reach back" to alter the variable's value for the parent [3]. If a declaration becomes "invalid at computed-value time" (for instance, because a referenced custom property contains an invalid value or causes a cycle), the property falls back to its initial or inherited value as if the declaration had been set to the unset keyword [1][6].
Citations:
- 1: https://www.w3.org/TR/css-variables/
- 2: https://drafts.csswg.org/css-variables-2/
- 3: [css-variables][css-cascade] When are variables computed? w3c/csswg-drafts#2793
- 4: https://moderncss.dev/how-custom-property-values-are-computed/
- 5: https://stackoverflow.com/questions/54445031/css-custom-property-change-on-inherited-color-value-doesnt-work
- 6: [css-variables] Substitution of invalid variables into other variables w3c/csswg-drafts#4075
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository root:"
pwd
echo
echo "Locate workspace-app.css:"
if [ -f src/ui/workspace-app.css ]; then
echo "found src/ui/workspace-app.css"
wc -l src/ui/workspace-app.css
else
fd -a 'workspace-app.css' .
fi
echo
echo "Relevant token and icon definitions:"
sed -n '1,25p;39,90p;150,170p' src/ui/workspace-app.css 2>/dev/null || true
echo
echo "All --tool-accent / --tool-accent-soft usages:"
rg -n -- '--tool-accent|--tool-accent-soft' src/ui/workspace-app.css 2>/dev/null || true
echo
echo "Selector/order scan:"
rg -n -- '.tool-card[.<:=>]|tool-icon|tool-card-header-bg|tool-card-body-bg|tool-card-border' src/ui/workspace-app.css 2>/dev/null || true
echo
echo "Parse token definitions and usages into a concise summary:"
python3 - <<'PY'
from pathlib import Path
p = Path('src/ui/workspace-app.css')
if not p.exists():
print("missing src/ui/workspace-app.css")
raise SystemExit
lines = p.read_text().splitlines()
for i,l in enumerate(lines,1):
v=l.strip()
if "--tool-accent" in v or "--tool-accent-soft" in v:
print(f"{i}: {v}")
# simple selector extraction for sections containing these tokens
if any(x in l for x in [".tool-card", ".tool-icon"]):
print(f"selector {i}: {l.strip()}")
PYRepository: Waishnav/devspace
Length of output: 8443
Make --tool-accent-soft participate in tone overrides.
--tool-accent-soft is declared only on :root, so its inner var(--tool-accent) resolves to the neutral default. Descendants inherit that resolved value, while .tool-icon.color uses the tone-specific --tool-accent. This makes each tone’s .tool-icon background grey instead of tinted by its semantic accent.
Move the soft accent definition to the tone overrides, or compute it inline where it is used.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/workspace-app.css` around lines 11 - 12, Update the --tool-accent-soft
definition in the tone override rules so it is recomputed from each tone’s
--tool-accent value, rather than remaining fixed from :root. Preserve the
existing color-mix behavior and ensure .tool-icon.color receives the
corresponding semantic tint for every tone.
| .tool-title { | ||
| color: var(--color-text-primary, #f5f5f6); | ||
| font-size: var(--font-text-md-size, 16px); | ||
| font-weight: 500; | ||
| line-height: 1.25; | ||
| font-size: var(--font-text-sm-size, 14px); | ||
| font-weight: 550; | ||
| line-height: 1.3; | ||
| } | ||
|
|
||
| .tool-label { | ||
| overflow: hidden; | ||
| color: var(--color-text-secondary, #d6d6dc); | ||
| color: var(--color-text-tertiary, #a3a3aa); | ||
| font-family: var(--font-mono, ui-monospace, SFMono-Regular, monospace); | ||
| font-size: var(--font-text-sm-size, 13px); | ||
| line-height: 1.35; | ||
| font-size: var(--font-text-sm-size, 12px); | ||
| line-height: 1.4; | ||
| text-overflow: ellipsis; | ||
| white-space: nowrap; | ||
| } | ||
|
|
||
| .stats { | ||
| display: inline-flex; | ||
| gap: 6px; | ||
| gap: 5px; | ||
| align-items: center; | ||
| font-family: var(--font-mono, ui-monospace, SFMono-Regular, monospace); | ||
| font-size: var(--font-text-sm-size, 13px); | ||
| font-size: var(--font-text-sm-size, 12px); | ||
| font-variant-numeric: tabular-nums; | ||
| white-space: nowrap; | ||
| } | ||
|
|
||
| .header-meta { | ||
| color: var(--color-text-secondary, #d6d6dc); | ||
| font-size: var(--font-text-sm-size, 13px); | ||
| color: var(--color-text-tertiary, #a3a3aa); | ||
| font-size: var(--font-text-sm-size, 12px); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
One font-size token carries five different intended sizes.
--font-text-sm-size appears with fallback 14px for .tool-title at Line 180, 12px for .tool-label at Line 189 and .stats at Line 200, 13px for .review-diff-file-name at Line 750, 11px for .workspace-chip at Line 356 and .workspace-instruction-preview at Line 613, and 10px for .workspace-instruction-path at Line 580.
The fallbacks are the only thing that creates the type hierarchy. When a host defines --font-text-sm-size, every one of these declarations resolves to that single value. The title, the mono label, the file path, the chips, and the preview text then render at identical size, and the compact hierarchy this PR introduces disappears.
Use separate tokens for the separate roles, for example --font-text-sm-size, --font-text-xs-size, and a fixed size where no token applies.
Based on learnings from the coding guidelines: "Verify the actual user-consumption path, including packaged npm/npx usage, real MCP hosts, restart requirements, checkout/worktree modes, supported platforms, tool surfaces, widgets, and rendered artifacts".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/workspace-app.css` around lines 178 - 207, Replace the shared
--font-text-sm-size usage across the affected typography roles with distinct
sizing tokens or fixed values: preserve the 14px tool-title size, use the
appropriate smaller token for .tool-label and .stats, and separately maintain
the 13px, 11px, and 10px sizes used by .review-diff-file-name, .workspace-chip,
.workspace-instruction-preview, and .workspace-instruction-path. Ensure host
overrides cannot collapse these roles into one size.
Source: Coding guidelines
| white-space: pre-wrap; | ||
| word-break: break-word; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the deprecated word-break: break-word.
Stylelint reports declaration-property-value-keyword-no-deprecated on Line 616. The break-word keyword for word-break is deprecated. Use overflow-wrap: break-word, which is the standard property for this behavior.
🎨 Proposed fix
white-space: pre-wrap;
- word-break: break-word;
+ overflow-wrap: break-word;
}Note: .text-payload at Line 837 carries the same deprecated declaration. Stylelint did not flag it because that line is unchanged, but consider updating both for consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| white-space: pre-wrap; | |
| word-break: break-word; | |
| white-space: pre-wrap; | |
| overflow-wrap: break-word; | |
| } |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 616-616: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/workspace-app.css` around lines 615 - 616, Replace the deprecated
word-break: break-word declaration in the affected style block with
overflow-wrap: break-word while preserving white-space: pre-wrap. Also update
the matching deprecated declaration in .text-payload for consistency.
Source: Linters/SAST tools
| for (const file of loadedFiles) { | ||
| const path = file.path ?? "AGENTS.md"; | ||
| loaded.push({ | ||
| path, | ||
| content: file.content, | ||
| status: "loaded", | ||
| }); | ||
| loadedPaths.add(path); | ||
| } | ||
|
|
||
| const available: WorkspaceInstruction[] = []; | ||
| for (const file of availableFiles) { | ||
| const path = file.path ?? "Nested instructions"; | ||
| if (!loadedPaths.has(path)) available.push({ path, status: "available" }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not invent an AGENTS.md path for instruction files that report no path.
At Line 575, a loaded file with path === undefined becomes the literal path "AGENTS.md". The UI then shows that filename and sets the title to Loaded into the current workspace context: AGENTS.md, which asserts a path the host never reported.
The value also becomes the identity key. Line 672 writes it to dataset.instructionPath, and syncWorkspaceInstructionPreviews matches on it at Line 732. Two loaded files without a path collapse to the same key, so opening one preview opens both.
Use a display label that does not claim a path, and key previews by index.
Based on learnings from the coding guidelines: "Represent important behavior through schemas, types, checks, or explicit tool results rather than hidden prompt conventions." and "Preserve host and provider data unless DevSpace has a concrete reason to normalize it".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/workspace-app.tsx` around lines 574 - 588, Update the loaded-file
handling in the workspace instruction rendering flow to preserve an undefined
path instead of defaulting to “AGENTS.md”; use a neutral display label for
pathless files without presenting it as a host-reported path. Change preview
identity and synchronization in the relevant instruction rendering and
syncWorkspaceInstructionPreviews logic to use each file’s index, including
dataset.instructionPath and matching, so multiple pathless files remain
distinct.
Source: Coding guidelines
1 participant
Tool cards had become visually heavy and inconsistent, while workspace metadata and file-change details were harder to scan than they needed to be. This refines the card shell, spacing, semantic accents, payload scrolling, and workspace detail layout, including expandable skills and instruction files with inline previews.
Patch and review cards now classify file operations from the actual diff, keep repeated-path operations distinct, show rename source and destination paths, align multi-file rows, and open a single-file apply_patch diff immediately without expanding large multi-file patches by default.
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Style