feat: Enhance UX #160
feat: Enhance UX #160
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds tag rendering and click behavior to PostPage, a TagPosts listing page and Tags utility, a Social service with social-link wiring, related tests, minor UI skeleton/layout changes, alias/tsconfig updates for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PostPage
participant TagsUtil
participant ApiStore
participant SearchInput
Note over PostPage: Tag click interaction (new)
User->>PostPage: click tag link
PostPage->>TagsUtil: formatLabel(tagName)
PostPage->>ApiStore: setSearchTerm(formattedLabel)
PostPage->>SearchInput: dispatch "input" event and focus
SearchInput-->>User: focused with value
sequenceDiagram
participant Router
participant TagPostsPage
participant ApiStore
participant ArticleItemPartial
Note over TagPostsPage: Tag-based fetch flow (new)
Router->>TagPostsPage: route param change (tag)
TagPostsPage->>TagPostsPage: bump requestId (cancel stale)
TagPostsPage->>ApiStore: getPosts({ tag })
ApiStore-->>TagPostsPage: posts or error
alt success
TagPostsPage->>ArticleItemPartial: render posts
else error
TagPostsPage-->>TagPostsPage: set hasError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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 |
Summary of ChangesHello @gocanto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user experience on the post page by introducing a dedicated section for displaying post tags. It ensures a smoother visual experience during data loading by adding tag placeholders to the skeleton screen and improves the overall robustness of the application by adding specific data attributes for testing and comprehensive unit tests for tag rendering. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the display of post tags by adding them to the post page, including a skeleton loader for a better loading experience, and adding tests for the new functionality. The changes are well-implemented. I have two main suggestions for improvement. First, in PostPage.vue, the tags should be interactive links to improve user navigation, rather than static text. Second, the tests in PostPage.test.ts could be made more robust by adding a case to verify that the tags container is not rendered when a post has no tags. These changes will improve usability and test coverage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/pages/PostPage.test.ts (1)
135-162: Add regression test for posts without tags. Line 135 exercises the happy path, but thev-if="post.tags?.length"guard still isn't covered. The earlier review comment flagged this gap and it's still outstanding—please add a no-tag scenario so we catch regressions when the array is empty.+ it('does not render the tag container when a post has no tags', async () => { + getPost.mockResolvedValueOnce({ ...post, tags: [] }); + const wrapper = mountComponent(); + await flushPromises(); + expect(wrapper.find('[data-testid="post-tags"]').exists()).toBe(false); + });
🧹 Nitpick comments (3)
src/partials/ResumePageSkeletonPartial.vue (1)
91-91: Stylistic change unrelated to PR objectives.The parameter rename from
eventto_eventin thedefineEmitstype signature follows the convention of prefixing unused identifiers with an underscore. However, this change is unrelated to the stated PR objectives (tag rendering, post page skeleton enhancements, and test assertions). Consider whether incidental style changes should be included in feature-focused PRs or batched separately.src/pages/ResumePage.vue (1)
101-101: Consider removing unused variable entirely.The variable
_resumeSectionMinHeightsis not referenced anywhere in this file. While prefixing with an underscore indicates it's intentionally unused, consider removing the line entirely to reduce code clutter. If the method call is needed for side effects, add a comment explaining why.Additionally, this change is unrelated to the stated PR objectives (tag rendering and post page enhancements). Consider whether such cleanup should be in a separate PR.
src/support/tags.ts (1)
41-61: Consider minor optimization: check for empty tag first.The method correctly handles all state scenarios with appropriate messaging. However,
formatLabelis called at Line 42 before checking if the tag is empty at Line 43, which means the formatted label is computed but unused when there's no tag.Apply this diff to optimize:
static summaryFor(tag: string, state: TagSummaryState): string { - const label = this.formatLabel(tag); if (!tag) { return 'Select a tag to explore related posts.'; } + const label = this.formatLabel(tag); + if (state.isLoading) { return `Loading posts for ${label}…`; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.github/workflows/format-check.yml(1 hunks).github/workflows/format.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks).nvmrc(1 hunks)aliases.ts(1 hunks)src/pages/PostPage.vue(7 hunks)src/pages/ResumePage.vue(1 hunks)src/pages/TagPostsPage.vue(1 hunks)src/partials/ArticleItemPartial.vue(3 hunks)src/partials/PostPageSkeletonPartial.vue(1 hunks)src/partials/ResumePageSkeletonPartial.vue(1 hunks)src/partials/SideNavPartial.vue(4 hunks)src/partials/WidgetSocialPartial.vue(2 hunks)src/router.ts(1 hunks)src/support/social.ts(1 hunks)src/support/tags.ts(1 hunks)tests/pages/PostPage.test.ts(5 hunks)tests/pages/TagPostsPage.test.ts(1 hunks)tests/partials/SideNavPartial.test.ts(4 hunks)tests/support/lazy-loading.test.ts(1 hunks)tests/support/social.test.ts(1 hunks)tests/support/tags.test.ts(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/format.yml
🧰 Additional context used
🧬 Code graph analysis (6)
tests/pages/TagPostsPage.test.ts (2)
src/stores/api/store.ts (1)
getPosts(114-122)src/stores/api/http-error.ts (1)
debugError(29-36)
tests/pages/PostPage.test.ts (1)
src/stores/api/store.ts (2)
getPost(123-131)setSearchTerm(34-36)
tests/support/social.test.ts (1)
src/support/social.ts (1)
Social(18-80)
src/support/social.ts (2)
src/stores/api/store.ts (1)
useApiStore(28-133)src/stores/api/http-error.ts (1)
debugError(29-36)
tests/support/tags.test.ts (1)
src/support/tags.ts (2)
TagSummaryState(3-7)Tags(9-62)
tests/partials/SideNavPartial.test.ts (2)
src/stores/api/store.ts (1)
getSocial(87-95)src/stores/api/client.ts (1)
ApiResponse(24-27)
🔇 Additional comments (20)
.gitignore (1)
12-12: Sensible addition to ignore generated test coverage artifacts.Adding
coverage/aligns with the PR's testing focus and follows the established pattern of excluding build/generated directories. This prevents test coverage reports from being committed to version control..github/workflows/format-check.yml (1)
23-23: Clarify the Node version downgrade rationale.The version has been downgraded from 22.18.0 to 22.12.0. This change is consistent across workflows and
.nvmrc, but the PR description doesn't mention the rationale. Confirm whether this downgrade is intentional (e.g., to resolve dependency conflicts) or unintended..nvmrc (1)
1-1: Version downgrade requires justification.The local development Node version has been downgraded to match the workflow changes. Confirm whether this addresses a specific compatibility concern or is a deliberate stability choice.
.github/workflows/test.yml (1)
15-15: Verify version downgrade doesn't introduce compatibility issues with test dependencies.Node version has been downgraded to 22.12.0. The PR objectives note that "
npm testcurrently fails becausevitestis not found in the environment." Confirm whether this version change is intended to resolve that issue or if the problem persists independently of the Node version.tests/support/lazy-loading.test.ts (1)
175-175: LGTM! Good cleanup for unused variable.The underscore prefix clearly indicates this variable is intentionally unused in this test case, which is appropriate since this test focuses on href changes and observer reinitialization rather than idle callback behavior.
src/partials/PostPageSkeletonPartial.vue (1)
22-36: LGTM! Navigation skeleton enhances loading UX.The new navigation skeleton provides visual continuity during page load, matching the breadcrumb/tag pattern referenced in the PR objectives. The styling is consistent with existing skeleton elements.
aliases.ts (1)
21-21: LGTM! Alias addition follows established patterns.The new
@supportalias enables clean imports from thesrc/supportdirectory, consistent with the existing alias structure.src/support/tags.ts (3)
12-23: LGTM! Robust parameter normalization.The method handles various input types appropriately, including arrays (taking the first element) and non-string types (returning empty string). The trimming ensures clean values.
25-32: LGTM! Clean label formatting logic.The method properly handles null/undefined/empty inputs with a default fallback and consistently formats non-empty tags with uppercase and
#prefix.
34-39: LGTM! Route construction is type-safe.The method correctly constructs a route object targeting the
TagPostsroute (defined in src/router.ts), maintaining type safety withRouteLocationRaw.tsconfig.json (1)
29-30: LGTM! Path alias configuration is correct.The new
@support/*path mapping enables TypeScript resolution for the support utilities and aligns with the alias defined in aliases.ts.src/partials/ArticleItemPartial.vue (1)
4-58: LGTM! Consistent migration to RouterLink component.The file correctly migrates from
router-linkto the PascalCaseRouterLinkcomponent with the appropriate import added at Line 66. All instances are updated consistently.src/router.ts (2)
25-25: LGTM! Named route improves navigation consistency.Adding the
Homename to the root route enables programmatic navigation using route names, consistent with other routes in the application.
33-37: LGTM! TagPosts route correctly configured.The new route properly captures the tag parameter and aligns with the
Tags.routeFormethod in src/support/tags.ts (which references the 'TagPosts' route name).tests/support/social.test.ts (2)
29-65: LGTM! Comprehensive test coverage for Social.fetch.The tests properly verify API success, empty data fallback, and error handling scenarios. The mock setup with
vi.hoistedandbeforeEachreset is well-structured.
67-97: LGTM! buildNavLinks tests verify filtering and defaults.The tests correctly validate platform filtering with an allowlist and default behavior. Including an unknown platform ('facebook') effectively tests the filtering logic.
tests/support/tags.test.ts (4)
10-27: LGTM! Comprehensive normalizeParam tests.The tests cover string trimming, array handling (both string and non-string elements), and unsupported types, ensuring robust input handling.
29-38: LGTM! formatLabel tests cover edge cases and formatting.The tests verify both the default label fallback for empty/null inputs and the correct uppercase formatting with
#prefix.
40-47: LGTM! routeFor test verifies route structure.The test correctly validates the generated route object. The single test case is sufficient given the method's simplicity.
49-95: LGTM! Comprehensive summaryFor tests cover all states.The tests thoroughly validate all scenarios: missing tag, loading, error, empty results, and post counts (including singular/plural handling). This ensures consistent user messaging across all states.
Summary
data-testidon the post tag collection to aid verificationTesting
Codex Task
Summary by CodeRabbit
New Features
Tests
Chores