feat(skills): show downloading state in Yours tab, remove toggle swit…#580
Merged
alchemistklk merged 3 commits intomainfrom Mar 26, 2026
Merged
feat(skills): show downloading state in Yours tab, remove toggle swit…#580alchemistklk merged 3 commits intomainfrom
alchemistklk merged 3 commits intomainfrom
Conversation
…ch, fix back navigation - Show actively downloading skills as cards at the top of the Yours tab with spinner and "Installing..." state, including correct sub-tab counts - Replace toggle switch with simple Install/Uninstall buttons on skill cards - Persist skills page view state (tab, sub-tab, tag, search) in URL search params so back navigation restores the exact view - Fix skill detail back button to use history-aware navigation with fallback
📝 WalkthroughWalkthroughAdds a URL-driven Skills view state module, wires it into the Skills page and Skill detail page for search-param-backed navigation and back-navigation handling, replaces switch-based install UI with explicit buttons, and adds tests for the new utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SkillsPage
participant Browser as BrowserHistory
participant DetailPage
participant URL as URLSearchParams
User->>SkillsPage: click skill (uses `detailTo`, includes state {fromSkillsList: true})
SkillsPage->>Browser: push detail route (path + URLSearchParams)
Note right of Browser: history index increments
User->>DetailPage: view detail
User->>DetailPage: click Back
DetailPage->>Browser: inspect history.state, Browser.index
alt openedFromSkillsList && Browser.canGoBack
DetailPage->>Browser: navigate(delta: -1)
else
DetailPage->>SkillsPage: navigate(to: /workspace/skills + URLSearchParams, replace:true)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Deploying nexu-docs with
|
| Latest commit: |
b067eb9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0fac547f.nexu-docs.pages.dev |
| Branch Preview URL: | https://feat-skill-download-state.nexu-docs.pages.dev |
Contributor
Author
|
/cr |
|
✅ CR topic created in Feishu topic group Refly CR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a49a6f8506
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
nettee
approved these changes
Mar 26, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/web/src/pages/skills.tsx`:
- Around line 135-138: The Link component (to={detailTo}
state={createSkillDetailState()} draggable={false}) currently wraps the entire
Skill card including the install/uninstall buttons, which nests interactive
controls inside an anchor; refactor the JSX so the Link only wraps the navigable
content (title, description, header) and move the install/uninstall Button(s)
into a separate footer/container sibling to the Link (e.g., a card-footer div)
so they are not children of Link; ensure any handlers that used
stopPropagation/preventDefault are removed or adjusted and that accessible focus
order and keyboard behavior remain correct for the Link and the separate Button
components (install/uninstall).
- Around line 179-208: The new CTA/status text ("Installing…", "Uninstalling…",
"Install", "Uninstall") is hardcoded and skips localization; update the
component to use the existing translation hook/strings instead of literal
text—e.g., import/use useTranslation() (or accept translated props from
SkillsPage) and replace the hardcoded labels inside the conditional rendering
(the branches guarded by isQueueActive/isMutating/pendingAction and the
Install/Uninstall buttons that call handleInstall/handleUninstall) with the
appropriate t('install'), t('installing'), t('uninstall'), t('uninstalling') (or
the project’s existing translation keys) so all CTA/status copy is localized.
🪄 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: a6bd7077-987b-4d4e-ad16-e1c30f767d8b
📒 Files selected for processing (4)
apps/web/src/lib/skills-view-state.tsapps/web/src/pages/community-skill-detail.tsxapps/web/src/pages/skills.tsxapps/web/tests/skills-view-state.test.ts
…exists Queued skills without a catalog entry would 404 on the detail page. Render those cards as plain containers instead of links while installing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/src/pages/skills.tsx (1)
135-138:⚠️ Potential issue | 🟠 MajorSplit the footer actions out of the
Link.The footer still cancels click/key events inside a wrapping
Link, so the whole bottom row becomes a dead zone and the Install/Uninstall buttons remain nested interactive content inside an anchor. That leaves the earlier keyboard/screen-reader issue unresolved.Also applies to: 165-176, 185-209
🧹 Nitpick comments (1)
apps/web/src/pages/skills.tsx (1)
351-383: Consider precomputing slug lookup maps once.
yourSkillsListandskillSourceresolution now do repeatedfind()scans overallSkillsandinstalledSkills. With the page rerendering on each search-param update, this path trends quadratic as the catalog grows. A slug-keyedMapwould keep the list assembly/render path linear.Also applies to: 782-785
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/pages/skills.tsx` around lines 351 - 383, The code repeatedly calls allSkills.find(...) and installedSkills.find(...) when building installed, downloadingWithSource and elsewhere (see installed, downloadingWithSource, yourSkillsList, skillSource), causing O(n^2) behavior on rerenders; fix by creating a slug-keyed Map (e.g., const skillBySlug = new Map(allSkills.map(s => [s.slug, s]))) once before these mappings and use skillBySlug.get(slug) instead of find(), and likewise create installedBySlug for installedSkills if needed; update installed, downloadingWithSource, yourSkillsList and skillSource to use the precomputed maps for O(1) lookups.
🤖 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/web/src/pages/skills.tsx`:
- Around line 617-623: The onClick handler is clearing the hidden tab's URL
state (setting yoursSubTab and activeTag unconditionally) which loses the
inactive tab's last sub-view; update the handler that calls updateViewState so
it only changes topTab and does not overwrite yoursSubTab or activeTag for the
non-active tab—i.e., set topTab to tab.id and only modify yoursSubTab/activeTag
when you really intend to change that tab's own state (use conditional logic
based on tab.id instead of always setting yoursSubTab/null and activeTag),
preserving the inactive tab's params so the search-param flow in
skills-view-state (the logic keyed off topTab) can restore them.
---
Nitpick comments:
In `@apps/web/src/pages/skills.tsx`:
- Around line 351-383: The code repeatedly calls allSkills.find(...) and
installedSkills.find(...) when building installed, downloadingWithSource and
elsewhere (see installed, downloadingWithSource, yourSkillsList, skillSource),
causing O(n^2) behavior on rerenders; fix by creating a slug-keyed Map (e.g.,
const skillBySlug = new Map(allSkills.map(s => [s.slug, s]))) once before these
mappings and use skillBySlug.get(slug) instead of find(), and likewise create
installedBySlug for installedSkills if needed; update installed,
downloadingWithSource, yourSkillsList and skillSource to use the precomputed
maps for O(1) lookups.
🪄 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: 9f32efe4-8ce2-4ea8-b0b0-f91b53bc7a89
📒 Files selected for processing (1)
apps/web/src/pages/skills.tsx
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds a URL-driven Skills view state module, wires it into the Skills page and Skill detail page for search-param-backed navigation and back-navigation handling, replaces switch-based install UI with explicit buttons, and adds tests for the new utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SkillsPage
participant Browser as BrowserHistory
participant DetailPage
participant URL as URLSearchParams
User->>SkillsPage: click skill (uses `detailTo`, includes state {fromSkillsList: true})
SkillsPage->>Browser: push detail route (path + URLSearchParams)
Note right of Browser: history index increments
User->>DetailPage: view detail
User->>DetailPage: click Back
DetailPage->>Browser: inspect history.state, Browser.index
alt openedFromSkillsList && Browser.canGoBack
DetailPage->>Browser: navigate(delta: -1)
else
DetailPage->>SkillsPage: navigate(to: /workspace/skills + URLSearchParams, replace:true)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ch, fix back navigation
What
Why
How
Affected areas
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpassespnpm generate-typesrun (if API routes/schemas changed)anytypes introduced (useunknownwith narrowing)Screenshots / recordings
Notes for reviewers
Summary by CodeRabbit
New Features
Improvements
Tests