Don't convert Windows-1252 encoding

[Boy132]

https://discord.com/channels/1218730176297439332/1449137767614255104

[coderabbitai]

📝 Walkthrough

Walkthrough

Removes Windows-1252 from the accepted input encodings in mb_convert_encoding calls across two file editing components. The encoding list is narrowed to UTF-8, UTF-16, ISO-8859-1, and ASCII in both the admin server page and file editing page.

Changes

Cohort / File(s)	Summary
Character encoding normalization app/Filament/Admin/Resources/Servers/Pages/EditServer.php, app/Filament/Server/Resources/Files/Pages/EditFiles.php	Removed Windows-1252 from the list of accepted input encodings passed to mb_convert_encoding when converting file/log contents to UTF-8. EditFiles.php has two affected call sites in content loading logic.

Possibly related PRs

• Encode file contents to utf-8 #1896: Previously added multi-encoding conversion support including Windows-1252 to these same files; this PR removes Windows-1252 from the accepted encodings, reversing part of that change.

Pre-merge checks

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The description is extremely vague, containing only a Discord link with no explanation of the changes or rationale.	Replace the Discord link with a clear explanation of why Windows-1252 encoding support is being removed and what problem this solves.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: removing Windows-1252 encoding support from the mb_convert_encoding calls across two files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

app/Filament/Server/Resources/Files/Pages/EditFiles.php (1)

176-176: Acknowledge that removing Windows-1252 encoding detection is an intentional design decision, but consider documenting the rationale.

The commit message explicitly states "don't convert Windows-1252 encoding" (commit 2ee3467), confirming this is a deliberate change, not accidental. The encoding list ['UTF-8', 'UTF-16', 'ISO-8859-1', 'ASCII'] is now consistently applied across the codebase (also in EditServer.php:339).

While no Discord discussion context or test files could be located to explain the reasoning, the technical concern about Windows-1252 file corruption remains valid: users editing legacy Windows-1252 encoded files may experience character corruption if those files contain extended ASCII characters. Consider adding a code comment explaining why Windows-1252 was intentionally excluded, or document this change in the project's migration guide if it affects user workflows.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a391d21 and 2ee3467.

📒 Files selected for processing (2)

• app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1 hunks)
• app/Filament/Server/Resources/Files/Pages/EditFiles.php (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: notAreYouScared
Repo: pelican-dev/panel PR: 1671
File: lang/en/server/database.php:22-25
Timestamp: 2025-09-06T20:20:48.706Z
Learning: In the pelican-dev/panel project, translation workflows for non-English languages are handled in separate PRs and processes, so missing translations in other locales should be ignored during code reviews.

🧬 Code graph analysis (1)

app/Filament/Server/Resources/Files/Pages/EditFiles.php (1)

app/Http/Controllers/Api/Client/Servers/FileController.php (1)

• contents (69-81)

🔇 Additional comments (1)

app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)

339-339: Note: Windows-1252 removal is intentional but lacks backward compatibility verification.

The removal of Windows-1252 from the encoding detection list is confirmed as intentional (per commit 2ee34674 "don't convert Windows-1252 encoding"). However, this change:

• Is applied consistently across both encoding conversion functions (install logs and file contents)
• Has no associated test coverage or safeguards to detect encoding issues
• Contains no comments explaining the rationale or confirming impact assessment

The technical concerns about display issues for legacy Windows-encoded logs remain valid. Consider adding a comment referencing the Discord discussion rationale and verifying no backward compatibility issues exist for servers with existing Windows-1252 encoded logs.