feat: Oullin Widget + Dialog Component #103
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 introduces a new 'Oullin' information widget to the application's homepage. This widget offers a brief overview of 'Oullin' and allows users to delve deeper into its narrative through an interactive modal dialog. The changes ensure a new piece of content is prominently displayed and accessible, backed by new unit tests to validate its functionality and user experience. 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
|
|
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. WalkthroughIntroduces a new AccessibleDialog component and a WidgetOullinPartial widget, integrates the widget into HomePage, updates several imports to use a new @components alias, adjusts tests accordingly, updates site URLs in index.html, and expands test coverage patterns. Minor test variable renames and a small prop-binding tweak included. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant HP as HomePage
participant WO as WidgetOullinPartial
participant AD as AccessibleDialog
participant D as Document
U->>WO: Click "Read more"
WO->>AD: v-model set true (open)
AD->>D: Lock body scroll (optional)
AD->>AD: Focus first focusable element
AD-->>WO: emit open
U->>AD: Press Escape / Click Close / Backdrop (if allowed)
AD->>AD: Set v-model false (close)
AD->>D: Restore body scroll
AD->>U: Restore focus to trigger
AD-->>WO: emit close
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (14)
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 |
gocanto
changed the title
![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 adds the new Oullin information widget to the homepage, complete with a modal dialog and unit tests. The implementation is clean and follows good Vue practices, such as using <Teleport> for the modal.
My review focuses on two key areas for improvement:
- Accessibility: Enhancing the modal dialog to be more accessible by allowing it to be closed with the
Escapekey. I've also noted further accessibility enhancements like focus trapping for your consideration. - Test Robustness: Improving the unit test to use more stable selectors, making it less likely to break with future UI changes.
I've provided specific code suggestions for these points. Overall, great work on the new feature.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/partials/WidgetOullinPartial.vue (1)
59-63: Consider clarifying the spelling variant explanation.The phrase "whether it's spelled oullin or ollin, or another fun variant" may confuse readers since the organization consistently uses "Oullin" throughout. Consider simplifying to make the deliberate spelling choice clearer without implying multiple active variants.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/pages/HomePage.vue(2 hunks)src/partials/WidgetOullinPartial.vue(1 hunks)tests/partials/WidgetOullinPartial.test.ts(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.39.5)
tests/partials/WidgetOullinPartial.test.ts
[warning] 5-5: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 GitHub Actions: Unit Tests
tests/partials/WidgetOullinPartial.test.ts
[error] 1-1: Test failed with TypeError: Cannot read properties of undefined (reading 'trigger') during test 'opens and closes the dialog when interacting with the read more link'.
🪛 GitHub Check: vitest
tests/partials/WidgetOullinPartial.test.ts
[failure] 25-25: tests/partials/WidgetOullinPartial.test.ts > WidgetOullinPartial > opens and closes the dialog when interacting with the read more link
TypeError: Cannot read properties of undefined (reading 'trigger')
❯ tests/partials/WidgetOullinPartial.test.ts:25:21
🔇 Additional comments (5)
src/partials/WidgetOullinPartial.vue (2)
1-15: LGTM! Card teaser implementation is solid.The widget card structure, styling, and button accessibility (focus-visible ring, proper type attribute) are all well implemented.
71-95: LGTM! State management and transition styles are clean.The reactive state handling and fade transition implementation are straightforward and correct. Note that the handlers will need updates per the accessibility suggestions above.
src/pages/HomePage.vue (1)
28-28: LGTM! Component integration is correct.The new
WidgetOullinPartialis properly imported and positioned ahead of the sponsor widget as intended. The integration follows the existing pattern for sidebar widgets.Also applies to: 54-54
tests/partials/WidgetOullinPartial.test.ts (2)
10-15: LGTM! Intro text test is correctly implemented.The test properly verifies the rendered content and cleans up with
wrapper.unmount().
5-7: The static analysis XSS warning is a false positive here.The
document.body.innerHTML = ''assignment in test cleanup is safe and standard practice for resetting the DOM between tests. There's no user input or XSS risk in this context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/partials/WidgetOullinPartial.test.ts (1)
18-33: Consider using data-testid for more robust button selection.The test correctly handles the Teleported close button by querying
document.body, and the error handling is good. However, the index-based selector (document.querySelectorAll('button')[1]) remains fragile—if button order changes, the test breaks.As noted in past reviews, using
data-testidattributes provides more stable selectors:
- Add to
WidgetOullinPartial.vue:<button data-testid="oullin-read-more">Read more</button> <button data-testid="oullin-close-dialog" type="button">Close</button>
- Update the test:
- await wrapper.get('button').trigger('click'); + await wrapper.get('[data-testid="oullin-read-more"]').trigger('click'); expect(document.body.textContent).toContain('For anyone on the path of self-discovery, Ollin becomes a guide'); - const closeButton = document.querySelectorAll('button')[1]; + const closeButton = document.body.querySelector('[data-testid="oullin-close-dialog"]'); if (!closeButton) throw new Error('Close button not found'); closeButton.click();This makes the test resilient to UI changes while remaining clear about which buttons are being tested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/partials/WidgetOullinPartial.test.ts(1 hunks)tests/partials/WidgetSocialPartial.test.ts(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.39.5)
tests/partials/WidgetOullinPartial.test.ts
[warning] 6-6: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (2)
tests/partials/WidgetOullinPartial.test.ts (1)
11-16: LGTM!The intro text rendering test is well-structured and correctly validates the component's initial display content.
tests/partials/WidgetSocialPartial.test.ts (1)
29-29: Component text matches updated expectationThe anchor text in src/partials/WidgetSocialPartial.vue is 'Code & Projects', aligning with the test assertion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/partials/WidgetOullinPartial.vue (1)
73-151: Excellent accessibility implementation!The component now includes all essential accessibility features that were requested in previous reviews:
- ESC key closes the dialog (lines 110-113)
- Focus is trapped within the dialog with Tab/Shift+Tab cycling (lines 116-132)
- Focus is restored to the trigger button on close (lines 101-103)
- Body scroll is locked while open and restored on close (lines 138-139, 143)
- Proper cleanup on unmount (lines 147-150)
The manual focus trap implementation is correct and comprehensive.
Optional: Consider using a headless UI library for simpler maintenance.
While the current implementation is solid, using
@headlessui/vue's Dialog component would reduce the amount of manual accessibility code and provide battle-tested patterns. This is entirely optional—your current implementation works well.tests/partials/WidgetOullinPartial.test.ts (1)
11-95: Comprehensive test coverage for dialog interactions.The test suite thoroughly covers:
- Rendering and initial state
- Dialog opening/closing via button clicks
- Focus management (restoration and initial focus)
- Body scroll locking
- Escape key handling
- Focus trap with Tab cycling
All tests correctly handle the Teleport pattern by querying
document.bodydirectly.Optional: Use data-testid for more robust button selection.
Lines 26, 46, and 83 use
document.querySelectorAll('button')[1]to select the close button. While this works, it's fragile—if button order changes, tests break. Consider addingdata-testid="dialog-close"to the close button in the component:<button data-testid="dialog-close" type="button" class="ml-4 text-slate-400..." @click="closeDialog" >Then query it in tests:
const closeButton = document.querySelector('[data-testid="dialog-close"]');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/partials/WidgetOullinPartial.vue(1 hunks)tests/partials/WidgetOullinPartial.test.ts(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.39.5)
tests/partials/WidgetOullinPartial.test.ts
[warning] 6-6: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (1)
tests/partials/WidgetOullinPartial.test.ts (1)
6-9: Test cleanup is appropriate.The
afterEachhook properly resetsdocument.bodystate between tests. The static analysis warning aboutinnerHTMLis a false positive—this is safe test cleanup, not user input.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/partials/WidgetOullinPartial.test.ts (1)
58-73: Consider verifying focus restoration after Escape.The test verifies that Escape closes the dialog, but it doesn't check whether focus is restored to the trigger button (unlike the close button test at lines 35-56). If your component restores focus on Escape, this test should verify that behavior for consistency.
If focus restoration on Escape is implemented, apply this diff:
it('closes when pressing the Escape key', async () => { const wrapper = mount(WidgetOullinPartial, { attachTo: document.body }); + const triggerButton = wrapper.get('button'); + triggerButton.element.focus(); - await wrapper.get('button').trigger('click'); + await triggerButton.trigger('click'); await nextTick(); expect(document.body.textContent).toContain('For anyone on the path of self-discovery, Ollin becomes a guide'); const escapeEvent = new KeyboardEvent('keydown', { key: 'Escape' }); document.dispatchEvent(escapeEvent); await nextTick(); expect(document.body.textContent).not.toContain('For anyone on the path of self-discovery, Ollin becomes a guide'); + expect(document.activeElement).toBe(triggerButton.element); wrapper.unmount(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/partials/WidgetOullinPartial.test.ts(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.39.5)
tests/partials/WidgetOullinPartial.test.ts
[warning] 6-6: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (5)
tests/partials/WidgetOullinPartial.test.ts (5)
6-9: LGTM: Test cleanup is safe.The static analysis warning about
innerHTMLmodification is a false positive. ResettinginnerHTMLto an empty string in test teardown is safe and a common practice for cleaning up the DOM between tests.
12-17: LGTM: Basic rendering test is well-structured.The test properly verifies the component's intro text content and unmounts the wrapper for cleanup.
19-33: LGTM: Dialog interaction test properly addresses previous feedback.The test correctly uses
data-testidfor selecting the close button (addressing the previous review comment about brittle selectors) and properly queries the teleported content fromdocument.body. The async handling withnextTick()is appropriate for Vue's reactivity updates.
35-56: LGTM: Focus and scroll restoration test is thorough.The test comprehensively verifies:
- Body overflow style changes (scroll locking when dialog opens)
- Focus moves to close button when dialog opens
- Focus returns to trigger button when dialog closes
- Body overflow resets when dialog closes
This demonstrates good accessibility testing practices.
75-92: LGTM: Focus trapping test covers keyboard navigation.The test verifies that Tab and Shift+Tab keep focus within the dialog. Note that if the dialog contains only one focusable element (the close button), the test naturally passes because there's nowhere else for focus to go. If you add more focusable elements to the dialog in the future, ensure this test still validates the full focus trap cycle.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
gocanto
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/pages/HomePage.vue (1)
29-34: Consider using v-if/v-else for cleaner conditional structure.The current template-based approach works correctly, but Vue's v-if/v-else would be more idiomatic and slightly more efficient.
Apply this diff for a cleaner structure:
- <template v-if="isLoadingProfile || !profile"> - <WidgetSkillsSkeletonPartial /> - </template> - <template v-else> - <WidgetSkillsPartial :skills="profile.skills" /> - </template> + <WidgetSkillsSkeletonPartial v-if="isLoadingProfile || !profile" /> + <WidgetSkillsPartial v-else :skills="profile.skills" />src/partials/WidgetOullinPartial.vue (1)
22-29: Expose dialog intent/state on the triggerAdd
aria-haspopup="dialog"and bindaria-expandedto the dialog state so screen readers announce that the button opens a modal and whether it’s currently open.<button type="button" class="mt-4 inline-flex items-center text-sm font-medium text-fuchsia-600 transition hover:text-fuchsia-500 focus:outline-none focus-visible:ring-2 focus-visible:ring-fuchsia-500 focus-visible:ring-offset-2 dark:text-teal-400 dark:hover:text-teal-300 dark:focus-visible:ring-teal-500 cursor-pointer" data-testid="oullin-dialog-trigger" + aria-haspopup="dialog" + :aria-expanded="isDialogOpen" @click="openDialog" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
index.html(2 hunks)src/pages/AboutPage.vue(1 hunks)src/pages/HomePage.vue(2 hunks)src/pages/PostPage.vue(1 hunks)src/partials/ArticleItemPartial.vue(1 hunks)src/partials/ArticleItemSkeletonPartial.vue(0 hunks)src/partials/DialogPartial.vue(1 hunks)src/partials/WidgetOullinPartial.vue(1 hunks)tests/components/CoverImageLoader.test.ts(1 hunks)tests/partials/ArticleItemPartial.test.ts(1 hunks)tests/partials/WidgetOullinPartial.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/partials/ArticleItemSkeletonPartial.vue
🧰 Additional context used
🪛 ast-grep (0.39.5)
tests/partials/WidgetOullinPartial.test.ts
[warning] 6-6: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (8)
src/pages/AboutPage.vue (1)
95-95: LGTM! Consistent refactoring to Partial naming convention.The import path update aligns with the project-wide migration to the CoverImageLoaderPartial component. The change is straightforward and maintains existing functionality.
tests/components/CoverImageLoader.test.ts (1)
3-3: LGTM! Test updated to reference the Partial component.The import path correctly targets CoverImageLoaderPartial.vue while preserving all test logic and expectations.
src/pages/PostPage.vue (1)
139-139: LGTM! Aligns with the CoverImageLoaderPartial migration.The import path update is consistent with the project-wide refactoring to Partial components.
src/partials/ArticleItemPartial.vue (1)
66-66: LGTM! Consistent with the Partial component migration.The import path update aligns with the project-wide refactoring to CoverImageLoaderPartial.vue.
tests/partials/ArticleItemPartial.test.ts (1)
5-5: LGTM! Test updated to target CoverImageLoaderPartial.The import correctly references the Partial variant while maintaining all test coverage for the cover image loader functionality.
src/pages/HomePage.vue (1)
28-28: LGTM! WidgetOullinPartial successfully integrated.The new Oullin widget is correctly imported and placed in the sidebar before the profile widgets, as described in the PR objectives.
Also applies to: 58-58
tests/partials/WidgetOullinPartial.test.ts (2)
1-101: Excellent test coverage for the WidgetOullinPartial component!The test suite is well-structured and comprehensive:
- Correctly handles Teleported content by querying
document.bodyusingDOMWrapper- Uses stable
data-testidselectors (addressing past review feedback)- Covers accessibility features: focus management, focus trap, keyboard navigation
- Proper lifecycle cleanup with
unmount()andafterEachbody resetThe static analysis warning about
innerHTMLon line 6 is a false positive—clearing test state withdocument.body.innerHTML = ''is safe in a test context as there's no user input involved.
22-22: Well done using data-testid attributes for stable test selectors.Using
data-testidattributes (e.g.,oullin-dialog-trigger,oullin-dialog-close-button,oullin-dialog-oss-link) makes the tests resilient to component structure changes while maintaining clarity. This addresses the brittleness concerns from previous reviews.Also applies to: 26-26, 37-37, 45-45, 61-61, 78-78, 82-83
gocanto
deleted the
gocanto/add-widget-box-about-ollin-on-home-page-2025-10-06
branch
Summary
Summary by CodeRabbit
New Features
Tests
Chores