fix(desktop): revert broken file icon origin fix + bundle all icon sources#3218
Conversation
The generate script was only collecting icons from fileNames, fileExtensions, folderNames, and folderNamesExpanded — missing languageIds (12 icons), rootFolder defaults, and rootFolderNames. Also expand languageId-to-extension mappings for php, tex, matlab, and diff so they resolve without VS Code's language detection.
📝 WalkthroughWalkthroughThese changes enhance the desktop application's icon handling system by expanding the file icon generation script to support VS Code languageIds mappings and additional folder icon categories, while updating the icon asset resolver's base URL derivation logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 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 |
Greptile SummaryThis PR contains two independent fixes for the desktop file-icon system:
Key changes:
One minor concern: Confidence Score: 4/5Safe to merge — the revert fixes a real production crash, and the icon bundling improvements are additive with no regressions. Score of 4 reflects a clean, well-motivated production fix with only one minor P2 robustness suggestion that does not affect correctness today. generate-file-icons.ts warrants a quick look at the languageIdExtensionMap to confirm icon names match manifest.languageIds values. Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer (file://)
participant F as resolveFileIconAssetUrl
participant U as new URL()
participant FS as File System / SVG
R->>F: resolveFileIconAssetUrl("typescript")
F->>F: baseHref = location.href
Note over F: e.g. file:///app/index.html#/dashboard
F->>U: new URL("file-icons/typescript.svg", baseHref)
U-->>F: file:///app/file-icons/typescript.svg
F-->>R: "file:///app/file-icons/typescript.svg"
R->>FS: img src file:///app/file-icons/typescript.svg
FS-->>R: SVG bytes
Note over R,FS: Previously broken path (PR #3199)
R->>F: resolveFileIconAssetUrl("typescript")
F->>F: location.origin = null string for file URLs
F->>U: new URL("file-icons/typescript.svg", null)
U-->>F: TypeError: Invalid URL
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/scripts/generate-file-icons.ts`:
- Around line 70-77: The languageIdExtensionMap entry for MATLAB is incorrect:
replace the key "matlab" with the actual file extension "m" (i.e., update
languageIdExtensionMap so it contains m: "matlab" instead of matlab: "matlab")
so `.m` files map to the MATLAB icon; locate and edit the languageIdExtensionMap
constant to make this change.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33f1036f-8cfc-4868-ad73-f010b053a363
📒 Files selected for processing (2)
apps/desktop/scripts/generate-file-icons.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/FilesView/utils/resolveFileIconAssetUrl.ts
| const languageIdExtensionMap: Record<string, string> = { | ||
| ts: "typescript", | ||
| js: "javascript", | ||
| php: "php", | ||
| tex: "tex", | ||
| matlab: "matlab", | ||
| diff: "diff", | ||
| }; |
There was a problem hiding this comment.
Bug: MATLAB extension mapping should be m, not matlab.
The PR objective states: "Verify that .m (matlab) files show correct icons." However, the current mapping matlab: "matlab" would match .matlab files, not .m files—MATLAB's actual file extension.
🐛 Proposed fix
const languageIdExtensionMap: Record<string, string> = {
ts: "typescript",
js: "javascript",
php: "php",
tex: "tex",
- matlab: "matlab",
+ m: "matlab",
diff: "diff",
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/scripts/generate-file-icons.ts` around lines 70 - 77, The
languageIdExtensionMap entry for MATLAB is incorrect: replace the key "matlab"
with the actual file extension "m" (i.e., update languageIdExtensionMap so it
contains m: "matlab" instead of matlab: "matlab") so `.m` files map to the
MATLAB icon; locate and edit the languageIdExtensionMap constant to make this
change.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/scripts/generate-file-icons.ts">
<violation number="1" location="apps/desktop/scripts/generate-file-icons.ts:75">
P2: The map key is used as the file extension (`condensed.fileExtensions[ext]`), so `matlab: "matlab"` registers `.matlab` — not `.m`. MATLAB's actual file extension is `.m`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Map `.m` (not `.matlab`) to the matlab icon since that's the actual file extension. Also add `.patch` → diff (already covered by upstream).
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…urces (superset-sh#3218) * Revert "fix(desktop): resolve file icons from origin instead of href (superset-sh#3199)" This reverts commit 5578746. * fix(desktop): collect all icon sources in file icon generation The generate script was only collecting icons from fileNames, fileExtensions, folderNames, and folderNamesExpanded — missing languageIds (12 icons), rootFolder defaults, and rootFolderNames. Also expand languageId-to-extension mappings for php, tex, matlab, and diff so they resolve without VS Code's language detection. * fix(desktop): correct languageId extension mappings for matlab/diff Map `.m` (not `.matlab`) to the matlab icon since that's the actual file extension. Also add `.patch` → diff (already covered by upstream).
cherry-pick済み: - e728ebd feat(desktop): wire up missing hotkeys for v2 workspace (superset-sh#3190) - 1eddeb3 feat(desktop): git changes sidebar with resource-oriented API (superset-sh#3177) - 11ed4f8 V2 terminal env (superset-sh#3184) - 0c52ecc feat(desktop): pane context menus + binary tree layout (superset-sh#3196) - 5578746 fix(desktop): resolve file icons from origin instead of href (superset-sh#3199) - 5a1e5d1 feat(panes): prefer sibling pane when closing active pane (superset-sh#3198) - d670c4a V2 top bar: right sidebar toggle, org dropdown in sidebar, unified open-in button (superset-sh#3197) - 2573fa2 fix(desktop): remove macOS background-to-tray quit interception (superset-sh#3205) - 4a29342 feat: Superset CLI + CLI framework + Better Auth 1.5.6 (superset-sh#3194) - 700cd65 fix(desktop): revert broken file icon origin fix + bundle all icon sources (superset-sh#3218) フォーク独自対応: - cleanupMainWindowResources()をexit pathに移動維持 (#3205対応) - BROWSER_HARD_RELOAD/SEARCH_IN_FILESをv2 workspaceに配線 - BROWSER_RELOAD/HARD_RELOADのuseHotkey配線修正（リマップ対応） - ansi_up依存維持
1 participant
Summary
resolve file icons from origin instead of href) which broke production —file://URLs have anullorigin, causingnew URL()to throw. The originalhref-based resolution works correctly with hash routing.languageIds(12 previously missing icons likeangular,php,matlab,diff),rootFolder/rootFolderExpanded, androotFolderNames/rootFolderNamesExpanded.php,tex,matlab,diff) so these extensions resolve at runtime without VS Code's language detection.Result: 1055 → 1067 SVGs bundled, 1148 → 1152 extension mappings.
Test plan
bun devinapps/desktopand verify file icons render in the files sidebar.php,.tex,.m(matlab),.difffiles show correct iconsfile://protocol)Summary by cubic
Fixes file icon URL resolution in the desktop app by reverting to
href, so icons load infile://builds. Also bundles icons from all manifest sources and expands extension mappings (including.mand.patch) to cover missing icons.Bug Fixes
file://has a null origin and brokenew URL.location.href(with trailing slash) as the base for icon URLs.New Features
languageIds,rootFolder/rootFolderExpanded, androotFolderNames/rootFolderNamesExpanded.php,tex,matlab(.m), anddiff(.patch).Written for commit fd50607. Summary will update on new commits.
Summary by CodeRabbit