chore: Move header social links into sidebar navigation #120
Conversation
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 refactors the application's UI by relocating social media links from the main hero section to the sidebar navigation. This change aims to declutter the hero component and integrate social links more cohesively within the primary navigation structure. The update involves modifying the relevant Vue components, adjusting a utility file for social link configuration, and updating the corresponding test files to ensure functionality and maintain code quality. 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
|
|
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. WalkthroughThis pull request refactors social links management across components and the API store: removes hero social links rendering, adds social links to SideNav, introduces centralized fetchSocial action with caching in the API store, updates component fetching patterns to use the store, simplifies icon styling, and adapts tests to reflect the new architecture. Changes
Sequence DiagramsequenceDiagram
participant HeroPartial as Hero<br/>(removed)
participant SideNav as SideNav<br/>(added)
participant WidgetSocial as WidgetSocial
participant APIStore as API Store<br/>(centralized)
participant Backend as Backend
Note over HeroPartial,Backend: Before: Multiple components fetch independently
HeroPartial->>Backend: fetch social (removed)
WidgetSocial->>Backend: fetch social
Note over HeroPartial,Backend: After: Centralized store with caching
SideNav->>APIStore: onMounted → fetchSocial()
WidgetSocial->>APIStore: onMounted → fetchSocial()
APIStore->>APIStore: Check cache
alt Cache hit
APIStore-->>SideNav: Return cached data
APIStore-->>WidgetSocial: Return cached data
else Cache miss
APIStore->>Backend: getSocial()
Backend-->>APIStore: social data
APIStore->>APIStore: Store in state.social
APIStore-->>SideNav: Return fetched data
APIStore-->>WidgetSocial: Return fetched data
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes The changes span multiple files with a coherent refactoring pattern (centralized store-based social fetching). However, logic density is moderate with new caching logic, multiple affected components, and integration test additions requiring careful validation of store wiring and data flow across components. Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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 successfully moves the social media links from the hero section to the sidebar navigation. The changes simplify the hero component and correctly update the corresponding tests and utility functions. My main feedback is an architectural suggestion to improve data fetching for better maintainability and performance, by centralizing the social links data in the Pinia store instead of fetching it directly within the SideNavPartial component.
gocanto
changed the title
![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
🧹 Nitpick comments (3)
src/stores/api/store.ts (1)
98-112: Consider adding a loading flag to prevent race conditions.The caching logic works correctly for sequential calls. However, if multiple components call
fetchSocial()simultaneously before the first fetch completes, both will see an empty cache and trigger duplicate API calls.While this won't break functionality and may be rare in practice, consider adding a loading flag or promise cache for more robust concurrent access:
// In state: social: [], socialLoading: false, socialPromise: null as Promise<SocialResponse[]> | null, // In fetchSocial: async fetchSocial(force = false): Promise<SocialResponse[]> { if (!force && this.social.length > 0) { return this.social; } if (this.socialPromise) { return this.socialPromise; } this.socialPromise = (async () => { try { const response = await this.getSocial(); if (Array.isArray(response.data)) { this.social = response.data; } else { this.social = []; } return this.social; } finally { this.socialPromise = null; } })(); return this.socialPromise; }tests/partials/SideNavPartial.test.ts (2)
54-79: Consider handling theforceparameter in the mock.The mount helper is well-designed for testing both success and error scenarios. However, the
fetchSocialmock implementation doesn't accept theforceparameter that the actual store method supports (async fetchSocial(force = false)). While this works for current tests, it could cause confusion if future tests need to verify behavior whenforce=trueis passed.To make the mock more complete:
- const fetchSocialMock = vi.spyOn(apiStore, 'fetchSocial').mockImplementation(async () => { + const fetchSocialMock = vi.spyOn(apiStore, 'fetchSocial').mockImplementation(async (force = false) => { if (rejectWith) { throw rejectWith; } apiStore.social = social; return apiStore.social; });
102-118: Social links rendering test is thorough.The test verifies API calls, link rendering, styling (
fill-currentclass on SVGs), and the presence of the separator element. Usingrel="noopener noreferrer"as a selector works well for external social links.For improved maintainability, consider adding
data-testidattributes to social links and the separator in the component, then using those for selection:const socialLinks = wrapper.findAll('[data-testid="social-link"]'); const separator = wrapper.find('[data-testid="social-separator"]');This would make tests more resilient to styling changes while still verifying functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/partials/HeroPartial.vue(0 hunks)src/partials/SideNavPartial.vue(3 hunks)src/partials/WidgetSocialPartial.vue(1 hunks)src/stores/api/store.ts(3 hunks)src/support/social.ts(1 hunks)tests/partials/HeroPartial.test.ts(1 hunks)tests/partials/SideNavPartial.test.ts(2 hunks)tests/partials/WidgetSocialPartial.test.ts(2 hunks)tests/stores/api/store.test.ts(2 hunks)tests/support/header-social-links.test.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/partials/HeroPartial.vue
🧰 Additional context used
🧬 Code graph analysis (3)
src/stores/api/store.ts (2)
src/stores/api/response/social-response.ts (1)
SocialResponse(1-7)src/stores/api/response/index.ts (1)
SocialResponse(26-26)
tests/partials/SideNavPartial.test.ts (1)
src/stores/api/http-error.ts (1)
debugError(29-36)
tests/partials/WidgetSocialPartial.test.ts (1)
src/stores/api/store.ts (1)
useApiStore(29-150)
🔇 Additional comments (17)
tests/partials/HeroPartial.test.ts (1)
2-2: LGTM! Test appropriately simplified.The removal of social data mocking and async assertions aligns with the architectural shift to store-driven social data. The streamlined test now focuses solely on HeroPartial's core responsibility: rendering hero copy and the avatar component.
Also applies to: 7-12
tests/support/header-social-links.test.ts (1)
41-41: LGTM! Test expectations correctly updated.The assertions now properly expect the simplified static
iconClassvalue'fill-current'instead of the previous dynamic Tailwind class strings, aligning with the changes insrc/support/social.ts.Also applies to: 75-75, 96-96
src/partials/SideNavPartial.vue (3)
68-78: LGTM! Social links section well-implemented.The social links integration is clean with proper accessibility features:
- Separator with
aria-hidden="true"correctly prevents screen reader announcement- Screen reader labels provided via
sr-onlyspans- External links properly marked with
target="_blank" rel="noopener noreferrer"- Icons rendered with appropriate SVG attributes
88-98: LGTM! Store integration properly implemented.The component now correctly uses the centralized API store for social data management. The computed wrapper ensures reactivity, and
useHeaderSocialLinksappropriately transforms the data for rendering.This addresses the previous review concern about centralizing social data in the Pinia store.
110-116: LGTM! Data fetching properly implemented.The
onMountedhook correctly:
- Awaits
fetchSocial()which handles caching internally- Catches and logs errors via
debugError- Prevents redundant API calls through store-level caching
src/partials/WidgetSocialPartial.vue (1)
28-28: LGTM! Clean migration to store-based data.The refactoring correctly:
- Replaces local reactive state with a computed wrapper over
apiStore.social- Delegates data fetching to the centralized
fetchSocial()action- Maintains reactivity through the computed property
- Preserves error handling patterns
This aligns with the broader architectural shift toward centralized social data management.
Also applies to: 34-34, 38-38
src/support/social.ts (1)
50-50: LGTM! Icon class appropriately simplified.The change from complex Tailwind utility classes to the minimal
'fill-current'is appropriate for sidebar navigation context, where the parent link styles (.blog-side-nav-router-link-a-restingand.blog-side-nav-router-link-a-active) handle color states and transitions.Also applies to: 54-54
tests/stores/api/store.test.ts (3)
5-5: LGTM! Comprehensive caching test.The test properly verifies:
- Initial fetch calls the API and populates store state
- Subsequent calls return cached data without additional API calls
- Both returned value and store state match the expected data
Also applies to: 155-180
182-212: LGTM! Forced refetch correctly tested.The test validates that the
forceparameter bypasses the cache, making a fresh API call and updating the store state with new data. Both API calls and state transitions are properly verified.
214-218: LGTM! Error propagation properly tested.The test confirms that errors from the underlying
getSocial()call are correctly propagated to the caller via the mockedparseErrorfunction.src/stores/api/store.ts (1)
24-24: LGTM! Store state properly extended.The
socialproperty is correctly typed and initialized as an empty array, providing a clean initial state for the cached social data.Also applies to: 33-33
tests/partials/WidgetSocialPartial.test.ts (1)
4-4: LGTM! Test properly refactored to use real Pinia store.The test correctly:
- Creates and activates a Pinia instance
- Obtains the API store and mocks
fetchSocial()to simulate data population- Verifies the component calls
fetchSocial()on mount- Asserts rendered output reflects the store's social data
This aligns with the architectural shift toward centralized store-based social data management.
Also applies to: 7-7, 21-43
tests/partials/SideNavPartial.test.ts (5)
1-14: Test setup looks solid.The imports, Pinia integration, and mock configuration are well-structured. Using
vi.mocked()for type-safe mock references is a good practice.
16-31: Test data is comprehensive.The social payload matches the
SocialResponsetype and provides realistic test data with all required fields.
82-84: Good practice to reset mocks between tests.Using
beforeEachto clear thedebugErrormock ensures test isolation.
86-100: Existing avatar tests updated correctly.The tests properly use the new helper signature and maintain their original behavior.
120-126: Error handling test is effective.The test correctly verifies that
debugErroris called whenfetchSocialfails, ensuring proper error logging.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f0a17bfe84833396dd42f7a0c52967
Summary by CodeRabbit
New Features
Style
Refactor