fix(apps): require shipped default app icons#262
Conversation
Summary: - Add the Profile manifest icon mapping to the committed profile asset. - Tighten the default app manifest test so missing icon fields fail. - Replace a pre-existing bare catch flagged by the pattern scanner. Rationale: - Default app icons should resolve deterministically from shipped assets instead of relying on per-user regeneration paths. - The previous validation only caught invalid string icons, not missing icon fields. Tests: - node scripts/build-default-apps.mjs home/apps - bun run test -- tests/gateway/apps.test.ts tests/gateway/default-icons.test.ts - bun run typecheck - bun run check:patterns - bun run test
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes the
Confidence Score: 5/5All changes are additive, narrowly scoped, and backed by confirmed asset presence and new test coverage. The manifest fix is confirmed correct (profile.png exists), the tightened validation only catches genuinely missing icons, and the error-boundary-utils change is behaviorally identical to the original with a new test proving the fallback path. No logic changes affect runtime paths beyond error description. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Test: visit home/apps/**/matrix.json"] --> B{"manifest.icon is string?"}
B -- "No (new check)" --> C["push 'missing icon' → test FAILS"]
B -- "Yes" --> D{"shippedIcons.has(manifest.icon)?"}
D -- "No" --> E["push icon name → test FAILS"]
D -- "Yes" --> F["OK — continue"]
G["profile/matrix.json\n icon: 'profile'"] --> D
H["home/system/icons/profile.png"] --> I["shippedIcons Set"]
I --> D
Reviews (2): Last reviewed commit: "fix(shell): keep error fallback tied to ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch (stringifyError) { | ||
| if (stringifyError instanceof globalThis.Error) return stringifyError.name; | ||
| return typeof error; |
There was a problem hiding this comment.
When String(error) throws and the caught exception is an Error, the function now returns stringifyError.name (e.g. "TypeError") — which names the stringify failure, not the original error argument. A caller reading that string would reasonably assume they're learning about the original error. The previous fallback typeof error was agnostic about both, so the semantic shift is small and this code path is extremely rare in practice, but stringifyError.name could mislead diagnostics downstream if it ever fires.
Prompt To Fix With AI
This is a comment left during a code review.
Path: shell/src/lib/error-boundary-utils.ts
Line: 19-21
Comment:
**Fallback describes the stringify error, not the original error**
When `String(error)` throws and the caught exception is an `Error`, the function now returns `stringifyError.name` (e.g. `"TypeError"`) — which names the *stringify failure*, not the original `error` argument. A caller reading that string would reasonably assume they're learning about the original error. The previous fallback `typeof error` was agnostic about both, so the semantic shift is small and this code path is extremely rare in practice, but `stringifyError.name` could mislead diagnostics downstream if it ever fires.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary: - Add a regression test for describeUnknownError when string coercion itself throws. - Return the original value type in that fallback path instead of the stringify failure name. Rationale: - Greptile flagged that returning the stringify failure's Error.name can mislead diagnostics by implying it describes the original thrown value. - The generic typeof fallback preserves the previous agnostic behavior while still satisfying the no-bare-catch scanner rule. Tests: - bun run test -- tests/shell/error-boundary-utils.test.ts - node scripts/build-default-apps.mjs home/apps - bun run test -- tests/gateway/apps.test.ts tests/gateway/default-icons.test.ts tests/shell/error-boundary-utils.test.ts - bun run typecheck - bun run check:patterns - bun run test
1 participant
Summary
profileasset.iconfields fail, not only unknown icon strings.describeUnknownErrorfallback diagnostics tied to the original value type when string coercion itself throws, with a regression test for the Greptile feedback item.Tests
node scripts/build-default-apps.mjs home/appsbun run test -- tests/gateway/apps.test.ts tests/gateway/default-icons.test.ts tests/shell/error-boundary-utils.test.tsbun run test -- tests/shell/error-boundary-utils.test.tsbun run typecheckbun run check:patterns(warnings only)bun run testGeneric Icons Retained
home/apps/games/**default game manifests intentionally continue to use the sharedgame-centericon.home/apps/symphony/matrix.jsonintentionally continues to use the existing sharedcodeicon.Invariants
matrix.jsonicon slugs are canonical for launch metadata, and committed files underhome/system/icons/are canonical for shipped icon assets.