Skip to content

Fix search with Polish character#3808

Merged
ildyria merged 1 commit intomasterfrom
fix-polish-characters
Nov 15, 2025
Merged

Fix search with Polish character#3808
ildyria merged 1 commit intomasterfrom
fix-polish-characters

Conversation

@ildyria
Copy link
Copy Markdown
Member

@ildyria ildyria commented Nov 15, 2025

Summary by CodeRabbit

  • Refactor
    • Improved search service encoding implementation for enhanced code organization.

@ildyria ildyria requested a review from a team as a code owner November 15, 2025 12:32
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Nov 15, 2025

📝 Walkthrough

Walkthrough

The change extracts base64 encoding logic into a dedicated private helper function base64encode() that combines TextEncoder with btoa, replacing the direct btoa() call in the search service's API request.

Changes

Cohort / File(s) Summary
Helper function extraction
resources/js/services/search-service.ts
Adds private base64encode(str: string): string helper function using TextEncoder + btoa; replaces inline btoa(terms) with base64encode(terms) in search API call

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Simple refactoring with no logic changes
  • Single file modification
  • Straightforward helper function extraction

Poem

🐰 A helper hops into the code,
Base64 encoding finds its home,
From btoa's call, now clean and tidy,
Search terms dance through TextEncoder's light.

Pre-merge checks

✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ildyria ildyria enabled auto-merge (squash) November 15, 2025 12:32
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
resources/js/services/search-service.ts (1)

29-34: Implementation is correct; consider optional performance improvement.

The UTF-8 encoding approach correctly handles Unicode characters. For typical search terms, the current implementation is fine.

If you'd like to optimize for longer strings, consider this alternative:

 function base64encode(str: string): string {
 	const bytes = new TextEncoder().encode(str);
-	let binary = "";
-	bytes.forEach((b) => (binary += String.fromCharCode(b)));
-	return btoa(binary);
+	return btoa(String.fromCharCode(...bytes));
 }

Or for better compatibility with very long strings:

 function base64encode(str: string): string {
 	const bytes = new TextEncoder().encode(str);
-	let binary = "";
-	bytes.forEach((b) => (binary += String.fromCharCode(b)));
-	return btoa(binary);
+	return btoa(Array.from(bytes, (byte) => String.fromCharCode(byte)).join(""));
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b27b6c5 and 156eb5f.

📒 Files selected for processing (1)
  • resources/js/services/search-service.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3641
File: lang/no/settings.php:9-9
Timestamp: 2025-08-22T06:11:18.329Z
Learning: For lang/* translation files in the Lychee project: only review PHP-related issues (syntax, structure, etc.), not translation content, grammar, or language-related nitpicks. The maintainer ildyria has explicitly requested this approach.
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
  • GitHub Check: 3️⃣ PHP dist / 8.3 - sqlite
  • GitHub Check: 3️⃣ PHP dist / 8.4 - mariadb
  • GitHub Check: 3️⃣ PHP dist / 8.4 - sqlite
  • GitHub Check: 3️⃣ PHP dist / 8.3 - postgresql
  • GitHub Check: 3️⃣ PHP dist / 8.4 - postgresql
  • GitHub Check: 3️⃣ PHP dist / 8.3 - mariadb
  • GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Unit
  • GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Install
  • GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Feature_v2
  • GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Webshop
  • GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Webshop
  • GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Feature_v2
  • GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Webshop
  • GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Feature_v2
  • GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Unit
  • GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Unit
🔇 Additional comments (2)
resources/js/services/search-service.ts (2)

23-23: LGTM! Correctly fixes Unicode handling.

The change from btoa(terms) to base64encode(terms) properly addresses the limitation of btoa() with Unicode characters like Polish diacritics.


29-34: Verify other btoa() usages in application source code—concern is valid.

Your review correctly identifies a real issue: btoa() throws InvalidCharacterError when given Unicode characters outside Latin-1 (0x00–0xFF). The fix shown in search-service.ts—using TextEncoder to encode to UTF-8 bytes, then converting to a binary string before calling btoa()—is the correct and preferred modern approach.

However, the search script output provided shows only vendor code (minified Vue.js library). To complete verification, ensure the search covers all application source files to identify any other btoa() usages. The vendor code should not be modified. Any btoa() calls in your application code should either already handle Unicode safely or be updated with the same TextEncoder pattern.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.37%. Comparing base (fad833e) to head (156eb5f).
⚠️ Report is 4 commits behind head on master.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@d7415 d7415 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂️

@ildyria ildyria merged commit c7453ad into master Nov 15, 2025
45 checks passed
@ildyria ildyria deleted the fix-polish-characters branch November 15, 2025 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants