Add ability to search episodes displayed on HTML page - closes #75 #77
Conversation
Reviewer's Guide by SourceryThis pull request adds the ability to search episodes displayed on the HTML page. It introduces a search input field and a JavaScript function to filter episodes based on the search term. The search is case-insensitive and filters based on the text content of each episode section. Sequence diagram for episode search interactionsequenceDiagram
actor User
participant SearchInput
participant SearchJS
participant Episodes
User->>SearchInput: Types search term
SearchInput->>SearchJS: Triggers oninput event
SearchJS->>Episodes: Get all episode sections
SearchJS->>Episodes: Filter episodes by search term
SearchJS->>Episodes: Show/hide episodes based on matches
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe pull request modifies the Overcast to SQLite project by introducing a search functionality to the HTML output. This includes the addition of a search input field in Changes
Sequence DiagramsequenceDiagram
participant User
participant HTML Page
participant Search Function
User->>HTML Page: Load Page
User->>HTML Page: Enter Search Text
HTML Page->>Search Function: Trigger searchEps()
Search Function->>HTML Page: Filter Sections
Search Function-->>User: Display Matching Episodes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Hey @hbmartin - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider removing the space after 'file://' in the CLI output to ensure consistent URL parsing across systems
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
overcast_to_sqlite/cli.py
Outdated
| @@ -308,7 +308,7 @@ def html( | |||
| else: | |||
| html_output_path = Path(db_path).parent / "overcast-played.html" | |||
| generate_html_played(db_path, html_output_path) | |||
| print("📝Saved HTML to:", html_output_path.absolute()) | |||
| print("📝Saved HTML to: file://", html_output_path.absolute()) | |||
There was a problem hiding this comment.
issue (bug_risk): Remove extra space after file:// protocol
The extra space after 'file://' could cause issues with URL handling in some environments. Consider concatenating the strings directly.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
overcast_to_sqlite/html/search.js (1)
1-11: The search implementation works but could be enhanced.While the core functionality is correct, consider these improvements:
- Use
querySelectorAll('section')instead ofgetElementsByTagNamefor better performance- Show all sections when search string is empty
- Add keyboard navigation and ARIA attributes for accessibility
function searchEps(searchString) { - const divs = Array.from(document.getElementsByTagName('section')); + const divs = Array.from(document.querySelectorAll('section')); + if (!searchString.trim()) { + divs.forEach(div => div.style.display = 'flex'); + return; + } divs.forEach(div => { const matches = div.innerText.toLowerCase().includes(searchString.toLowerCase()); if (matches) { div.style.display = 'flex'; } else { div.style.display = 'none'; } }); }overcast_to_sqlite/html/index.html (1)
9-11: Add defer attribute to script tag for better performance.The script should be loaded with defer to prevent blocking page rendering.
<script> {script} </script> + <script defer> + {script} + </script>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
overcast_to_sqlite/cli.py(1 hunks)overcast_to_sqlite/html/episode.html(1 hunks)overcast_to_sqlite/html/index.html(1 hunks)overcast_to_sqlite/html/page.py(1 hunks)overcast_to_sqlite/html/search.js(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- overcast_to_sqlite/html/episode.html
- overcast_to_sqlite/cli.py
🔇 Additional comments (1)
pyproject.toml (1)
12-12: LGTM!The addition of
*.jsto package data is correct and necessary for including the search functionality in the distribution.
| @@ -33,6 +33,7 @@ def generate_html_played(db_path: str, html_output_path: Path) -> None: | |||
| page_vars = { | |||
| "title": "Recently Played", | |||
| "style": Path(this_dir / "mvp.css").read_text(), | |||
| "script": Path(this_dir / "search.js").read_text(), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file read operations.
The script file read operation should handle potential IOErrors to prevent crashes if the file is missing or unreadable.
- "script": Path(this_dir / "search.js").read_text(),
+ "script": Path(this_dir / "search.js").read_text() if (this_dir / "search.js").exists() else "",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "script": Path(this_dir / "search.js").read_text(), | |
| "script": Path(this_dir / "search.js").read_text() if (this_dir / "search.js").exists() else "", |
overcast_to_sqlite/html/index.html
Outdated
| <p><small>Made with ❤️ by your friendly neighborhood hbmartin</small></p> | ||
| </footer> | ||
| <main> | ||
| <input type="text" oninput="searchEps(this.value)" placeholder="Search"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance search input accessibility and UX.
The search input needs a label and better styling for accessibility and user experience.
- <input type="text" oninput="searchEps(this.value)" placeholder="Search">
+ <div class="search-container">
+ <label for="episode-search">Search Episodes:</label>
+ <input
+ type="text"
+ id="episode-search"
+ oninput="searchEps(this.value)"
+ placeholder="Search episodes..."
+ aria-label="Search episodes"
+ >
+ </div>Add this CSS to the existing style section:
.search-container {
margin: 1rem 0;
}
.search-container input {
width: 100%;
padding: 0.5rem;
font-size: 1rem;
border: 1px solid #ccc;
border-radius: 4px;
}
Summary by Sourcery
New Features:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates