spec: File Tree icon themes (#9731)#10013
Conversation
Adds product.md and tech.md for issue warpdotdev#9731: support icon themes for the Project Explorer. V1 scope (per reporter's pre-bounding): - Settings → Appearance dropdown picks between bundled themes - Two bundled themes: `default` (byte-equivalent to today's hardcoded mapping in app/src/code/icon.rs) and `material` (alternative) - Documented JSON theme format (VS Code File Icon Theme-style): iconDefinitions + fileExtensions + fileNames + folderNames + folderNamesExpanded + file/folder/folderExpanded defaults - Resolution order: filename → extension → folder name → fallback - Read-only theme catalog (themes loaded from bundled assets only) Out of V1 (tracked as follow-ups): - User-supplied theme files from disk - In-app theme editor, per-file overrides, theme marketplace - Language-ID-based resolution - Per-icon tint flag (V1 hard-codes the existing tint set for md/sh) - Nerd Font codepoint format alternative 10 testable behavior invariants, each mapped to a test layer in tech.md. Invariant 1 enforces byte-equivalence between the new default theme and today's hardcoded mapping — zero visual regression for existing users. Tech spec maps to existing call sites: app/src/code/icon.rs preserves its function signature; app/src/code/file_tree/view/render.rs gets a new folder_icon helper; settings entry plugs into the existing define_settings_group! macro.
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this spec-only pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This spec PR defines product and technical requirements for configurable File Tree icon themes, including bundled theme JSON, settings UI, registry lookup, and tests. The scope generally matches the linked issue, but several spec contradictions would lead to an implementation that fails the stated behavior invariants.
Concerns
- The tech spec simultaneously says
render.rsneeds no changes and later requires folder-render changes for themed folder icons. - The folder lookup pseudocode does not implement the documented per-folder expanded-to-collapsed fallback and would fail invariant 6 when
folderNamesExpandedis partially populated. - Missing asset handling is specified as fallthrough, but the registry pseudocode returns icon paths without any existence validation.
- The default catalog example maps JavaScript extensions to the TypeScript key even though the current implementation uses
javascript.svg, undermining the zero-regression requirement.
Security
- The spec introduces community-contributed bundled SVG assets but leaves SVG safety constraints to a follow-up; add V1 acceptance criteria for safe bundled SVGs so the new asset format has a clear trust boundary.
Verdict
Found: 0 critical, 4 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| | Path | Role | | ||
| |---|---| | ||
| | `app/src/code/icon.rs` | The closed `match` to be replaced by the theme registry. The function signature `icon_from_file_path(path: &str, appearance: &Appearance) -> Option<Box<dyn Element>>` is preserved to keep the call site untouched. | | ||
| | `app/src/code/file_tree/view/render.rs` | Sole call site. Calls `icon_from_file_path` per row; falls back to `Icon::File`/`Icon::Folder` on `None`. No changes required to this file. | |
There was a problem hiding this comment.
render.rs needs no changes, but Section 4 requires changing folder rendering to support themed folders; update this table so implementers do not skip the render-site change.
| let folder_map = if is_expanded | ||
| && !self.file.folder_names_expanded.is_empty() | ||
| { | ||
| &self.file.folder_names_expanded | ||
| } else { | ||
| &self.file.folder_names | ||
| }; |
There was a problem hiding this comment.
folderNames when folderNamesExpanded is empty globally; if the expanded map has entries for other folders, a folder declared only in folderNames falls through to the generic default, contradicting invariant 6. Specify per-name fallback from folderNamesExpanded.get(name) to folderNames.get(name).
| &self.file.folder_names | ||
| }; | ||
| if let Some(key) = folder_map.get(name) { | ||
| if let Some(def) = self.file.icon_definitions.get(key) { |
There was a problem hiding this comment.
resolve returns the referenced iconPath as soon as the key exists, so nonexistent assets cannot fall through to the next lookup step as product.md requires. Specify load-time validation that removes invalid definitions, or a runtime asset-existence check before returning.
| "rs": "rs", | ||
| "json": "json", | ||
| "ts": "ts", "tsx": "ts", | ||
| "js": "ts", "jsx": "ts", |
There was a problem hiding this comment.
js/jsx to the ts key contradicts the zero-regression default theme requirement and the current icon.rs JavaScript mapping; make the concrete catalog example byte-equivalent instead of leaving a contradictory note later.
| ## Goals | ||
|
|
||
| - A user can switch the File Tree icon theme in **Settings → Appearance** without restarting Warp. | ||
| - The theme format is documented so a community PR adding a third theme is mechanical: drop a JSON file plus its referenced SVGs under the bundled assets directory and add an entry to the theme registry. |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] Since V1 allows community PRs to add bundled SVGs, include V1 asset constraints here: SVGs must be local bundled assets with no scripts, remote references, or unsafe external dependencies, rather than deferring SVG constraints entirely to follow-up docs.
1 participant
Adds a product+tech spec for #9731: icon-theme support for the Project Explorer / File Tree, replacing today's hardcoded mapping in
app/src/code/icon.rswith a small, data-driven theme registry that the community can extend via PRs.Files
specs/GH9731/product.md(140 lines) — V1 scope, user experience, configuration shape, 10 testable behavior invariantsspecs/GH9731/tech.md(270 lines) — module layout, theme model + registry types, integration with the existingicon_from_file_pathcall site, settings entry, bundled-theme catalog, end-to-end flowTotal: 538 insertions, 0 deletions. No code changes — this is a spec PR.
V1 scope (per reporter's pre-bounding in #9731)
default(byte-equivalent to today's mapping; zero visual regression) andmaterial(alternative).iconDefinitions+fileExtensions+fileNames+folderNames+folderNamesExpanded+file/folder/folderExpandeddefaults.Out of V1 (tracked as follow-ups)
tint: trueflag in the theme JSON (V1 hard-codes the existing tint set used formdandsh).Why this spec
ready-to-spec, no existing PR claims it.app/src/code/, one new settings entry, the existingicon_from_file_pathsignature is preserved, only one render-site change to handle folder rendering.Open questions for maintainers
appearance.file_tree_icon_theme. Confirm the right Rust setting group (AppearanceSettingsvs a newFileTreeSettings) at implementation time.docs/file-tree-icon-themes.mdpackaging. Recommend it ships in the same release as the implementation PR but does not block this spec.Happy to iterate on the design or tighten the V1 scope further.