Feature/search keyboard shortcut#4397
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds a ChangesEnhanced Search Experience
Supporting Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as frontend.ts (DOMContentLoaded)
participant initSearchShortcut
participant initSearchShortcutBadge
participant attachAutocomplete
participant openSearchModal
Frontend->>initSearchShortcut: init()
Frontend->>initSearchShortcutBadge: init()
Frontend->>attachAutocomplete: handleAutoComplete() → attachAutocomplete(input)
User->>initSearchShortcut: keydown Cmd+K
alt inline input exists
initSearchShortcut->>attachAutocomplete: input.focus() / scrollIntoView
else no inline input
initSearchShortcut->>openSearchModal: openSearchModal()
openSearchModal->>attachAutocomplete: attachAutocomplete(modalInput)
end
User->>attachAutocomplete: types query
rect rgba(59, 130, 246, 0.5)
note over attachAutocomplete: query non-empty
attachAutocomplete->>attachAutocomplete: fetchAutoCompleteData(query)
end
rect rgba(16, 185, 129, 0.5)
note over attachAutocomplete: query empty
attachAutocomplete->>attachAutocomplete: buildEmptyStateItems() → recent + popular
end
attachAutocomplete-->>User: rendered suggestion list
User->>attachAutocomplete: selects item
attachAutocomplete->>attachAutocomplete: addRecentSearch(term)
attachAutocomplete-->>User: navigate to item.url
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
phpmyfaq/assets/src/utils/platform.ts (1)
26-26: ⚡ Quick winMove shortcut hint labels into the translation system.
'⌘ K'/'Ctrl K'are hard-coded in TypeScript on Line 26. Please read them via translation keys to stay consistent with i18n rules and locale-specific labels (e.g., Ctrl naming).As per coding guidelines, "Do not hard-code strings — use the translation system".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/assets/src/utils/platform.ts` at line 26, The getShortcutHintLabel function currently returns hard-coded strings '⌘ K' and 'Ctrl K' instead of using the translation system. Replace the hard-coded string literals with translation key lookups using your i18n system. Create appropriate translation keys for both the Mac platform shortcut hint (using ⌘) and the non-Mac platform shortcut hint (using Ctrl), then call your translation function within getShortcutHintLabel to fetch and return the locale-specific labels based on the platform condition.Source: Coding guidelines
phpmyfaq/assets/src/utils/platform.test.ts (1)
14-37: ⚡ Quick winAdd coverage for
navigator.userAgentData.platformbranch and empty-string fallback.Current tests only cover
navigator.platform. Please add cases whereuserAgentData.platformis set and where it is'', so the fallback behavior is locked in and prevents regressions inisMacPlatform().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/assets/src/utils/platform.test.ts` around lines 14 - 37, The current test suite for isMacPlatform() and getShortcutHintLabel() only covers the navigator.platform path via setPlatform(). Add new test cases to cover the navigator.userAgentData.platform branch by testing scenarios where userAgentData.platform contains macOS indicators (like 'MacOS') and non-macOS values, as well as test cases where the platform value is an empty string to verify the fallback behavior functions correctly. This ensures all code branches in isMacPlatform() are exercised and prevents regressions when either platform detection method is used.phpmyfaq/assets/src/interfaces/PopularSearch.ts (1)
4-4: ⚡ Quick winNormalize API ambiguity at the boundary instead of exporting
number | stringdownstream.Using
number | stringin the shared interface propagates backend inconsistency to all consumers. Prefer a raw API type plus a normalized app-facing type (count: number) converted in the API client.Based on learnings: "Isolate external dependencies behind an interface you own, so they can be swapped or mocked at the boundary" and "Make illegal states unrepresentable — encode invariants in types and constructors instead of re-validating them everywhere."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/assets/src/interfaces/PopularSearch.ts` at line 4, The PopularSearch interface exports a `number` property typed as `number | string`, which forces all consumers to handle both types. Instead, create two types: keep a raw API response type (RawPopularSearch or similar) that matches the backend response with the union type, and normalize the PopularSearch interface to only have `number: number`. In the API client code that fetches popular searches, convert the raw API response to the normalized PopularSearch type by coercing the number field to an actual number, ensuring all downstream consumers see a consistent numeric type.Source: Learnings
phpmyfaq/assets/src/api/popularSearches.test.ts (1)
13-43: ⚡ Quick winAdd a malformed-success-payload test case for boundary robustness.
Please add a case where fetch returns
200with an invalid JSON shape and assert the fallback behavior ([]or filtered-normalized output). Current tests don’t lock this contract.Based on learnings: "Always write tests for new features and bug fixes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/assets/src/api/popularSearches.test.ts` around lines 13 - 43, Add a new test case to the fetchPopularSearches test suite that verifies the behavior when the fetch returns a 200 status code but with an invalid or malformed JSON payload that doesn't conform to the expected data shape. Mock the response with a 200 status but provide data that is either malformed JSON, missing expected fields, or has incorrect types. Assert that the fetchPopularSearches function returns an empty array or applies appropriate filtering and normalization to handle this edge case, ensuring the function gracefully handles payload mismatches without breaking.Source: Learnings
phpmyfaq/assets/src/interfaces/suggestionItem.ts (1)
5-11: ⚡ Quick winModel
SuggestionItemas a discriminated union to prevent invalid variant shapes.The current optional-property interface allows impossible states. Define per-
typevariants (e.g.,result,recent,popular,empty) with required fields per variant so misuse is caught at compile time.Based on learnings: "Make illegal states unrepresentable — encode invariants in types and constructors instead of re-validating them everywhere."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/assets/src/interfaces/suggestionItem.ts` around lines 5 - 11, The SuggestionItem interface allows invalid state combinations through excessive optional properties. Refactor it into a discriminated union by creating separate interface variants for each SuggestionType value (such as result, recent, popular, empty), where each variant has a literal type field and its own required fields (for example, the result variant requires question and category while empty requires none). Then export a union type that combines all variants so TypeScript enforces that only valid field combinations are allowed per type.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phpmyfaq/assets/src/api/popularSearches.ts`:
- Line 34: The function at line 34 unsafely casts the JSON response directly to
PopularSearchResponse using the `as` operator without any runtime validation.
This allows malformed or incorrect data from the backend to propagate into the
autocomplete rendering. Add validation logic before the return statement to
validate the array items and their shape, verify that numeric fields are
properly coerced to numbers, and throw an error or return a default value if
validation fails. This ensures that only well-formed data matching the expected
PopularSearchResponse structure is returned from this function.
In `@phpmyfaq/assets/src/search/autocomplete.ts`:
- Around line 103-127: The fetch callback does not handle errors from
fetchAutoCompleteData(), which throws on non-OK responses and causes the
rejection to bubble up, preventing update() from being called and leaving the
autocomplete UI stuck. Wrap the fetchAutoCompleteData(query) call in a try-catch
block to gracefully handle errors. When an error is caught, ensure update() is
still invoked with an appropriate fallback state (such as an empty suggestion
list or similar) so the UI can recover from transient network or backend
failures.
In `@phpmyfaq/assets/src/search/recentSearches.ts`:
- Around line 26-27: The getRecentSearches() function needs to normalize and cap
parsed entries before returning them. In the return statement where you
currently filter for string types, you must also trim whitespace from each item
using the trim() method, filter out any empty strings after trimming, and cap
the resulting array to a reasonable maximum length (consider defining a constant
for this limit). This ensures that externally modified localStorage cannot
inject blank entries or arbitrarily large arrays into the autocomplete
rendering.
In `@phpmyfaq/assets/src/search/searchModal.ts`:
- Around line 30-31: The searchModal.ts file contains hard-coded user-facing
strings in the placeholder and aria-label properties instead of using the
translation system, which violates the coding guidelines. Replace the hard-coded
string 'Search …' in the placeholder property and the hard-coded string 'Search'
in the aria-label property with appropriate translation function calls that
retrieve these strings from the project's translation system, ensuring
consistency with the rest of the search UI localization.
In `@tests/bootstrap.php`:
- Around line 96-107: Add an `is_readable()` check before the dynamic include
statement for `$databaseConfigPath` in the code block around the `include
$databaseConfigPath;` line. This will satisfy the static analysis warning about
dynamic file inclusion by validating that the configuration file is readable in
addition to the existing `file_exists()` check. This follows the same defensive
pattern already implemented in DatabaseConfiguration.php.
---
Nitpick comments:
In `@phpmyfaq/assets/src/api/popularSearches.test.ts`:
- Around line 13-43: Add a new test case to the fetchPopularSearches test suite
that verifies the behavior when the fetch returns a 200 status code but with an
invalid or malformed JSON payload that doesn't conform to the expected data
shape. Mock the response with a 200 status but provide data that is either
malformed JSON, missing expected fields, or has incorrect types. Assert that the
fetchPopularSearches function returns an empty array or applies appropriate
filtering and normalization to handle this edge case, ensuring the function
gracefully handles payload mismatches without breaking.
In `@phpmyfaq/assets/src/interfaces/PopularSearch.ts`:
- Line 4: The PopularSearch interface exports a `number` property typed as
`number | string`, which forces all consumers to handle both types. Instead,
create two types: keep a raw API response type (RawPopularSearch or similar)
that matches the backend response with the union type, and normalize the
PopularSearch interface to only have `number: number`. In the API client code
that fetches popular searches, convert the raw API response to the normalized
PopularSearch type by coercing the number field to an actual number, ensuring
all downstream consumers see a consistent numeric type.
In `@phpmyfaq/assets/src/interfaces/suggestionItem.ts`:
- Around line 5-11: The SuggestionItem interface allows invalid state
combinations through excessive optional properties. Refactor it into a
discriminated union by creating separate interface variants for each
SuggestionType value (such as result, recent, popular, empty), where each
variant has a literal type field and its own required fields (for example, the
result variant requires question and category while empty requires none). Then
export a union type that combines all variants so TypeScript enforces that only
valid field combinations are allowed per type.
In `@phpmyfaq/assets/src/utils/platform.test.ts`:
- Around line 14-37: The current test suite for isMacPlatform() and
getShortcutHintLabel() only covers the navigator.platform path via
setPlatform(). Add new test cases to cover the navigator.userAgentData.platform
branch by testing scenarios where userAgentData.platform contains macOS
indicators (like 'MacOS') and non-macOS values, as well as test cases where the
platform value is an empty string to verify the fallback behavior functions
correctly. This ensures all code branches in isMacPlatform() are exercised and
prevents regressions when either platform detection method is used.
In `@phpmyfaq/assets/src/utils/platform.ts`:
- Line 26: The getShortcutHintLabel function currently returns hard-coded
strings '⌘ K' and 'Ctrl K' instead of using the translation system. Replace the
hard-coded string literals with translation key lookups using your i18n system.
Create appropriate translation keys for both the Mac platform shortcut hint
(using ⌘) and the non-Mac platform shortcut hint (using Ctrl), then call your
translation function within getShortcutHintLabel to fetch and return the
locale-specific labels based on the platform condition.
🪄 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: c687b525-dbdc-41a6-9ee7-a4f059c5b1bd
📒 Files selected for processing (34)
AGENTS.mdphpmyfaq/assets/scss/layout/_autocomplete.scssphpmyfaq/assets/src/api/index.tsphpmyfaq/assets/src/api/popularSearches.test.tsphpmyfaq/assets/src/api/popularSearches.tsphpmyfaq/assets/src/frontend.test.tsphpmyfaq/assets/src/frontend.tsphpmyfaq/assets/src/interfaces/PopularSearch.tsphpmyfaq/assets/src/interfaces/index.tsphpmyfaq/assets/src/interfaces/suggestionItem.tsphpmyfaq/assets/src/search/autocomplete.test.tsphpmyfaq/assets/src/search/autocomplete.tsphpmyfaq/assets/src/search/highlight.test.tsphpmyfaq/assets/src/search/highlight.tsphpmyfaq/assets/src/search/index.tsphpmyfaq/assets/src/search/recentSearches.test.tsphpmyfaq/assets/src/search/recentSearches.tsphpmyfaq/assets/src/search/searchModal.test.tsphpmyfaq/assets/src/search/searchModal.tsphpmyfaq/assets/src/search/searchShortcut.test.tsphpmyfaq/assets/src/search/searchShortcut.tsphpmyfaq/assets/src/search/searchShortcutBadge.test.tsphpmyfaq/assets/src/search/searchShortcutBadge.tsphpmyfaq/assets/src/utils/index.tsphpmyfaq/assets/src/utils/platform.test.tsphpmyfaq/assets/src/utils/platform.tsphpmyfaq/assets/templates/default/index.twigphpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/PopularSearchesController.phpphpmyfaq/src/services.phpphpmyfaq/translations/language_de.phpphpmyfaq/translations/language_en.phptests/bootstrap.phptests/phpMyFAQ/Controller/Frontend/Api/PopularSearchesControllerDirectTest.phptests/phpMyFAQ/Controller/Frontend/Api/PopularSearchesControllerWebTest.php
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
1 participant
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.
Summary by CodeRabbit
Release Notes