fix: UI polish, auth flow, and webkit compatibility#1556
fix: UI polish, auth flow, and webkit compatibility#1556
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR polishes the skills browsing UI (filters, counts, category dropdown), improves Safari/WebKit compatibility, and refactors the sign-in UX into a reusable component used across multiple routes.
Changes:
- Added a reusable
SignInButtonand updated multiple signed-out empty states to use it. - Introduced skill categories (with icons) and updated skills browsing/count behavior (including “Other” category logic).
- Added WebKit/Safari compatibility patches (including a Vite transform for Ark/Arktype
in-operator usage) and expanded test coverage.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Adds a Vite pre-transform plugin to patch Ark/Arktype code for Safari/WebKit compatibility. |
| src/routes/u/$handle.tsx | Uses the shared SignInButton inside a signed-out empty state. |
| src/routes/stars.tsx | Uses the shared SignInButton inside a signed-out empty state. |
| src/routes/skills/index.tsx | Updates the displayed counts to reflect filtering and reduce “blank page” flashes. |
| src/routes/skills/-useSkillsBrowseModel.ts | Adds “Other” category filtering and category keyword support for browsing logic. |
| src/routes/skills/-SkillsToolbar.tsx | Adds category dropdown with icons; shows an active capability-tag chip. |
| src/routes/settings.tsx | Uses the shared SignInButton in the signed-out settings view. |
| src/routes/publish-skill.tsx | Replaces inline auth actions with SignInButton; improves scrollIntoView guard. |
| src/routes/import.tsx | Uses the shared SignInButton in the signed-out import view. |
| src/routes/dashboard.tsx | Uses the shared SignInButton in the signed-out dashboard view. |
| src/routes/cli/auth.tsx | Refactors CLI auth sign-in to use SignInButton and removes duplicated redirect logic. |
| src/routes/-settings.test.tsx | Updates settings tests to mock auth actions for SignInButton usage. |
| src/lib/packageApi.ts | Replaces in checks with hasOwnProperty for WebKit-safe property guards. |
| src/lib/hasOwnProperty.ts | Introduces a shared hasOwnProperty type-guard helper. |
| src/lib/convexError.ts | Uses hasOwnProperty for safer error shape inspection. |
| src/lib/categories.ts | Introduces skill category definitions and flattened keyword list. |
| src/components/UserBadge.tsx | Uses hasOwnProperty for safer type narrowing on user-like objects. |
| src/components/SkillDetailPage.tsx | Uses hasOwnProperty for safer error formatting. |
| src/components/SignInButton.tsx | Adds reusable sign-in button with shared error handling + redirect behavior. |
| src/components/SignInButton.test.tsx | Adds unit tests covering default redirect and error-handling behavior. |
| src/components/Header.tsx | Switches header sign-in to SignInButton and removes duplicated auth error logic. |
| src/tests/skills-index.test.tsx | Adds test ensuring capability-tag chip is shown and can be cleared. |
| src/tests/skill-detail-page.test.tsx | Adds a mock to stabilize skill detail page tests with new dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const CATEGORY_ICONS: Record<string, React.ReactNode> = { | ||
| "mcp-tools": <Plug size={13} />, | ||
| prompts: <MessageSquare size={13} />, | ||
| workflows: <GitBranch size={13} />, | ||
| "dev-tools": <Wrench size={13} />, | ||
| data: <Database size={13} />, | ||
| security: <Shield size={13} />, | ||
| automation: <Zap size={13} />, | ||
| other: <Package size={13} />, | ||
| }; |
There was a problem hiding this comment.
React.ReactNode is used for CATEGORY_ICONS and FilterChip props, but React isn’t imported in this file. With the automatic JSX runtime, JSX works, but the React namespace for types will be undefined and TypeScript will fail to compile. Import type React from "react" or switch these annotations to ReactNode via import type { ReactNode } from "react".
| </span> | ||
| )} | ||
| <span className="ml-2 text-lg font-normal text-[color:var(--ink-soft)] opacity-70"> | ||
| ({model.hasQuery || model.highlightedOnly || model.nonSuspiciousOnly |
There was a problem hiding this comment.
The header count switches to model.sorted.length only for query/highlighted/nonSuspicious filters, but not when capabilityTag is active. If a capability tag filter is set, the count in the title will still show the total skills rather than the filtered count. Include model.capabilityTag in this condition so the displayed count matches the active filters.
| ({model.hasQuery || model.highlightedOnly || model.nonSuspiciousOnly | |
| ({model.hasQuery || model.highlightedOnly || model.nonSuspiciousOnly || model.capabilityTag |
| const browseResponse = response as PackageCatalogBrowseResponse; | ||
| return { | ||
| items: | ||
| "results" in response ? response.results.map((entry) => entry.package) : response.items, | ||
| nextCursor: "results" in response ? null : response.nextCursor, | ||
| items: browseResponse.items, | ||
| nextCursor: browseResponse.nextCursor, | ||
| }; |
There was a problem hiding this comment.
response is cast to PackageCatalogBrowseResponse without validating that items is present/array-shaped. If the API ever returns an unexpected shape (or results exists but isn’t an array), this will return undefined fields and likely fail later. Consider using hasOwnProperty(response, "items") + Array.isArray(...) (and validating nextCursor) and throwing a PackageApiError (or a clear fallback) when the shape is unrecognized.
| for (const [from, to] of fix.replacements) { | ||
| if (!nextCode.includes(from)) { | ||
| this.error(`Expected to patch ${from} in ${normalizedId}`); | ||
| } | ||
| nextCode = nextCode.replaceAll(from, to); | ||
| } | ||
|
|
There was a problem hiding this comment.
This Vite transform hard-fails the build via this.error(...) if any expected source substring is missing. Since this is patching code in dependencies, a minor upstream version change or different bundling output can break builds unexpectedly. Consider making the patch more resilient (e.g., tolerate missing patterns with a warning, or gate the patch to a known dependency version / specific build mode).
| for (const [from, to] of fix.replacements) { | |
| if (!nextCode.includes(from)) { | |
| this.error(`Expected to patch ${from} in ${normalizedId}`); | |
| } | |
| nextCode = nextCode.replaceAll(from, to); | |
| } | |
| let patchedAny = false; | |
| const missingPatterns: string[] = []; | |
| for (const [from, to] of fix.replacements) { | |
| if (!nextCode.includes(from)) { | |
| missingPatterns.push(from); | |
| continue; | |
| } | |
| nextCode = nextCode.replaceAll(from, to); | |
| patchedAny = true; | |
| } | |
| if (missingPatterns.length > 0) { | |
| this.warn( | |
| `Skipped ${missingPatterns.length} ark safari patch replacement(s) in ${normalizedId}: ${missingPatterns.join(", ")}`, | |
| ); | |
| } | |
| if (!patchedAny) return null; |
Greptile SummaryThis PR delivers UI polish to the skills browser (squircle filter chips, category dropdown with icons, per-filter skill-count updates), extracts
Confidence Score: 4/5Safe to merge with minor issues — one edge-case count bug for the "Other" category and a CSS dark-mode regression on toolbar elements. Changes are well-scoped with good test coverage added for SignInButton, the settings route, and skill-detail. The webkit compatibility work is thorough and intentional. The two flagged issues are narrow in scope and do not affect core functionality.
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/routes/skills/index.tsx
Line: 82-84
Comment:
**Header count shows total when "Other" category is active**
When the "Other" category is selected from the dropdown, `query === "__other__"` causes `isOtherCategory = true` and `hasQuery = false` in the model (by design). Since neither `highlightedOnly` nor `nonSuspiciousOnly` are set, the header count falls through to `totalSkillsText` — the unfiltered total — even though `model.sorted` is already client-side filtered to only "Other" items.
Every other active filter (Staff Picks, Clean only, any keyword category) correctly shows `model.sorted.length`. Only the "Other" branch is missed.
```suggestion
({model.hasQuery || model.highlightedOnly || model.nonSuspiciousOnly || model.query === "__other__"
? model.sorted.length.toLocaleString("en-US")
: totalSkillsText ?? "…"})
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/routes/skills/-SkillsToolbar.tsx
Line: 205
Comment:
**Hardcoded rgba values break dark mode**
The view-toggle container (line 205) and the `FilterChip` base style (line 252) both use hardcoded light-mode values:
- `bg-[rgba(255,255,255,0.94)]` — near-opaque white
- `border-[rgba(29,59,78,0.22)]` — fixed dark-ink opacity
These replaced the previous `var(--surface)` / `var(--line)` CSS custom properties that adapt to the active theme. In dark mode the chips and toggle will render with a bright white background instead of the expected dark surface color.
Consider restoring the CSS variable equivalents or adding `dark:` Tailwind variants so the toolbar respects the user's theme.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: recompute other skills category fil..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ({model.hasQuery || model.highlightedOnly || model.nonSuspiciousOnly | ||
| ? model.sorted.length.toLocaleString("en-US") | ||
| : totalSkillsText ?? "…"}) |
There was a problem hiding this comment.
When the "Other" category is selected from the dropdown, query === "__other__" causes isOtherCategory = true and hasQuery = false in the model (by design). Since neither highlightedOnly nor nonSuspiciousOnly are set, the header count falls through to totalSkillsText — the unfiltered total — even though model.sorted is already client-side filtered to only "Other" items.
Every other active filter (Staff Picks, Clean only, any keyword category) correctly shows model.sorted.length. Only the "Other" branch is missed.
| ({model.hasQuery || model.highlightedOnly || model.nonSuspiciousOnly | |
| ? model.sorted.length.toLocaleString("en-US") | |
| : totalSkillsText ?? "…"}) | |
| ({model.hasQuery || model.highlightedOnly || model.nonSuspiciousOnly || model.query === "__other__" | |
| ? model.sorted.length.toLocaleString("en-US") | |
| : totalSkillsText ?? "…"}) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/skills/index.tsx
Line: 82-84
Comment:
**Header count shows total when "Other" category is active**
When the "Other" category is selected from the dropdown, `query === "__other__"` causes `isOtherCategory = true` and `hasQuery = false` in the model (by design). Since neither `highlightedOnly` nor `nonSuspiciousOnly` are set, the header count falls through to `totalSkillsText` — the unfiltered total — even though `model.sorted` is already client-side filtered to only "Other" items.
Every other active filter (Staff Picks, Clean only, any keyword category) correctly shows `model.sorted.length`. Only the "Other" branch is missed.
```suggestion
({model.hasQuery || model.highlightedOnly || model.nonSuspiciousOnly || model.query === "__other__"
? model.sorted.length.toLocaleString("en-US")
: totalSkillsText ?? "…"})
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| {/* View toggle */} | ||
| <div className="inline-flex items-center rounded-[var(--radius-pill)] border border-[color:var(--line)] bg-[color:var(--surface)] p-0.5"> | ||
| <div className="inline-flex items-center rounded-[var(--radius-sm)] border border-[rgba(29,59,78,0.22)] bg-[rgba(255,255,255,0.94)] p-0.5"> |
There was a problem hiding this comment.
The view-toggle container (line 205) and the FilterChip base style (line 252) both use hardcoded light-mode values:
bg-[rgba(255,255,255,0.94)]— near-opaque whiteborder-[rgba(29,59,78,0.22)]— fixed dark-ink opacity
These replaced the previous var(--surface) / var(--line) CSS custom properties that adapt to the active theme. In dark mode the chips and toggle will render with a bright white background instead of the expected dark surface color.
Consider restoring the CSS variable equivalents or adding dark: Tailwind variants so the toolbar respects the user's theme.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/skills/-SkillsToolbar.tsx
Line: 205
Comment:
**Hardcoded rgba values break dark mode**
The view-toggle container (line 205) and the `FilterChip` base style (line 252) both use hardcoded light-mode values:
- `bg-[rgba(255,255,255,0.94)]` — near-opaque white
- `border-[rgba(29,59,78,0.22)]` — fixed dark-ink opacity
These replaced the previous `var(--surface)` / `var(--line)` CSS custom properties that adapt to the active theme. In dark mode the chips and toggle will render with a bright white background instead of the expected dark surface color.
Consider restoring the CSS variable equivalents or adding `dark:` Tailwind variants so the toolbar respects the user's theme.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d96cddfdbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (cat.slug === "other") { | ||
| onQueryChange("__other__"); | ||
| } else if (cat.keywords[0]) { |
There was a problem hiding this comment.
When the user picks the Other category, this code writes "__other__" into the shared query state, and that state is also bound directly to the search input. In practice, users will see the raw sentinel in the text box and URL (for example /skills?q=__other__), which is an internal implementation detail and creates confusing UI/state behavior around search and filtering. Keep category selection separate from the free-text query (or map this sentinel back to an empty/display-safe query value before rendering).
Useful? React with 👍 / 👎.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| </span> | ||
| )} | ||
| <span className="ml-2 text-lg font-normal text-[color:var(--ink-soft)] opacity-70"> | ||
| ({model.hasQuery || model.highlightedOnly || model.nonSuspiciousOnly |
There was a problem hiding this comment.
When the "Other" category is selected, the header displays total skills count instead of the filtered count
Summary
Test plan