修复若干问题,主要是Linux适配引起的回归问题 - #70
Conversation
它们在 c411998 中被再次地错误添加进来。
收窄了isPhotino的判断条件。 同时优化了`return Forbid()`的使用(会引起500)
…on:closeModal)被误判为非AbortError。
同时在Linux下,隐藏兼容模式的按钮。
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
审阅者指南该 PR 通过调整音频解码行为、平台检测、本地化字符串、前端 API/类型以及导入/导出行为,并从解决方案中清理未使用的项目,修复了主要由 Linux 适配引起的回归问题。 AppVersionResult 的 platform/export 与 ignoreGapless 选项时序图sequenceDiagram
actor User
participant Frontend
participant AppVersionController
participant StaticSettings
User->>Frontend: load ImportChartButton
Frontend->>AppVersionController: GetAppVersion()
AppVersionController->>StaticSettings: read gameVersion, Config.Export, CurrentLocale
AppVersionController-->>Frontend: AppVersionResult(version, gameVersion, license, hardwareAcceleration, h264Encoder, locale, platform, export)
Frontend->>Frontend: store version in refs.version
Frontend->>Frontend: render ignoreGapless only when version.platform == Windows
文件级变更
提示与命令与 Sourcery 交互
自定义你的体验访问你的控制面板 以:
获取帮助Original review guide in EnglishReviewer's GuidePR fixes regressions mainly caused by Linux adaptation by adjusting audio decoding behavior, platform detection, localized strings, frontend API/types, and import/export behavior, plus cleaning unused projects from the solution. Sequence diagram for AppVersionResult platform/export and ignoreGapless optionsequenceDiagram
actor User
participant Frontend
participant AppVersionController
participant StaticSettings
User->>Frontend: load ImportChartButton
Frontend->>AppVersionController: GetAppVersion()
AppVersionController->>StaticSettings: read gameVersion, Config.Export, CurrentLocale
AppVersionController-->>Frontend: AppVersionResult(version, gameVersion, license, hardwareAcceleration, h264Encoder, locale, platform, export)
Frontend->>Frontend: store version in refs.version
Frontend->>Frontend: render ignoreGapless only when version.platform == Windows
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - 我在这里给出一些高层次的反馈:
- 在
AppVersionController中,Platform字段是通过OperatingSystem.IsWindows()计算出来的,而这个调用本身已经处于特定平台的编译分支中;你可以通过在 WINDOWS 分支中直接返回常量 "Windows",在非 Windows 分支中返回 "Linux" 来简化逻辑并让意图更加清晰。 - 新增的
importErrors.ts中的isAbortError辅助函数现在接受any类型,并且会把任何name === 'AbortError'的对象当作中止错误;建议收窄可接受的类型(例如 DOMException | Error),或者增加更严格的结构检查,以避免把任意对象误分类为中止错误。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- In `AppVersionController` the `Platform` field is computed via `OperatingSystem.IsWindows()` inside already platform-specific compilation branches; you could simplify and make intent clearer by returning constant "Windows" in the WINDOWS branch and "Linux" in the non-Windows branch.
- The new `isAbortError` helper in `importErrors.ts` now accepts `any` and treats any object with `name === 'AbortError'` as an abort; consider narrowing the accepted types (e.g. DOMException | Error) or adding a stricter shape check to avoid misclassifying arbitrary objects.帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进之后的代码审查。
Original comment in English
Hey - I've left some high level feedback:
- In
AppVersionControllerthePlatformfield is computed viaOperatingSystem.IsWindows()inside already platform-specific compilation branches; you could simplify and make intent clearer by returning constant "Windows" in the WINDOWS branch and "Linux" in the non-Windows branch. - The new
isAbortErrorhelper inimportErrors.tsnow acceptsanyand treats any object withname === 'AbortError'as an abort; consider narrowing the accepted types (e.g. DOMException | Error) or adding a stricter shape check to avoid misclassifying arbitrary objects.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AppVersionController` the `Platform` field is computed via `OperatingSystem.IsWindows()` inside already platform-specific compilation branches; you could simplify and make intent clearer by returning constant "Windows" in the WINDOWS branch and "Linux" in the non-Windows branch.
- The new `isAbortError` helper in `importErrors.ts` now accepts `any` and treats any object with `name === 'AbortError'` as an abort; consider narrowing the accepted types (e.g. DOMException | Error) or adding a stricter shape check to avoid misclassifying arbitrary objects.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
All reported issues were addressed across 15 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
否则,压缩 WAV(例如 ADPCM)现在会在导入时失败
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Auto-approved: 主要变更修复资源泄露、平台兼容性限制、导出模式错误处理、宿主检测改进以及项目构建调整,均为明确的 bug 修复和清理,没有需要人工审批的行为权衡。
Re-trigger cubic
| { | ||
| // 仅本地桌面场景,export / 远程模式下禁止 | ||
| if (StaticSettings.Config.Export) return Forbid(); | ||
| if (StaticSettings.Config.Export) return StatusCode(StatusCodes.Status403Forbidden); |
There was a problem hiding this comment.
诶,那这样的话,export+应用内打开是不是会炸
(至少 Windows 是有 export+应用内打开的操作的,我不记得 Linux 做没做了)
There was a problem hiding this comment.
首先,原本的代码return Forbid();是 26530b1 中引入的,并不是我写的;我推测这么做的目的可能是,远程情况下浏览器和后端不运行在一台机器上,很多情况下后端是游戏专机之类的、可能连完整的鼠标键盘都没,因此从后端的机器调用本机的文件选择API、弹出文件选择窗口,前端那边也是看不到,没有意义的。但实际上,在(修改后的)正常的逻辑之下,理论上Export模式应该一定调用的是浏览器侧的文件选择API(因为isPhotino必定工作在非export模式下?见下面讨论),所以这个代码其实只是个Guard;
MaiChartManager/MaiChartManager/Front/src/utils/pickDirectory.ts
Lines 23 to 51 in 2a32f30
其次,我这里的修改只是把return Forbid();改成了return StatusCode(StatusCodes.Status403Forbidden);,因为测试发现,return Forbid();由于一些配置上的原因,无法返回403,而是抛异常导致500。(AI分析如下图)

最后,直接回答您的”export+应用内打开是不是会炸“的问题。分两个方面:
- Windows而言不会炸(我已实际测试)。这是因为即便是export+应用内打开,应用内打开调的也是WebView2,而Windows上的WebView2的实现是完整的,带有正常的浏览器侧的文件选择API。因此文件选择窗口最终实际上还是由前端WebView打开的,根本不会调用到您引用的这个后端函数。
- Linux上。其实最简单的办法当然也是前端打开,但是根据
pickDirectory.ts中的注释,之所以不这么做是因为WebKitGTK 即使暴露了 window.showDirectoryPicker,其实现也有问题(调用后访问 handle 会抛 "The string did not match the expected pattern"),所以只好退而求其次用后端文件选择作为workaround。所以,问题的关键就是isPhotino是不是保证一定非export模式。 如果是的话则没问题;不然的话,则这里确实是个问题(但也是从 26530b1 就引入了的)
There was a problem hiding this comment.
好的 我觉得我应该检查一下,就不让 Linux 在 export 模式下再在应用内打开好了
There was a problem hiding this comment.
好呀,我觉得可以,因为「选择」这个行为本身也一定是在本机上完成的,因为弹窗只能在本机上弹出来
不过我是希望,如果不是 localhost 的访问,或者不是 photino 而是本机 chromium 之类的,还是用像原先一样的浏览器端直接选择文件夹,而不需要走这个绕一圈(
There was a problem hiding this comment.
好的 我觉得我应该检查一下,就不让 Linux 在 export 模式下再在应用内打开好了
讲一点我的观察,供您参考:
- 在完全不考虑安全性的情况下:这个“export模式下返回403”的逻辑其实没有意义、不会命中,干脆可以去掉。因为在正常的程序逻辑下,后端文件选择函数
PickImportFolderAPI一定是前端通过pickViaBackend触发的。阅读相关代码容易知道,它一定是在localhost访问的情况下才会触发的(isPhotino内也隐含了isLocalhost的判断的),所以正常情况下并不可能出现“用户在其他机器上远程访问,却在后端机器上弹出了个对话框”的情况。
MaiChartManager/MaiChartManager/Front/src/utils/pickDirectory.ts
Lines 23 to 51 in 2a32f30
- 所以AI原本的顾虑其实是安全性问题。也就是恶意用户,并不通过我们的前端,而是直接强行请求API的情况。
- 后端提供的文件操作API中除了有
PickImportFolder这种只是弹对话框、本身无害的,但也有ListImportDir和ReadImportFile这种可以实现任意文件读取的。 确实可能有明显的安全风险。
- 后端提供的文件操作API中除了有
综上以上大概是我对情况的分析和总结,具体怎么改看您
There was a problem hiding this comment.
好呀,我觉得可以,因为「选择」这个行为本身也一定是在本机上完成的,因为弹窗只能在本机上弹出来 不过我是希望,如果不是 localhost 的访问,或者不是 photino 而是本机 chromium 之类的,还是用像原先一样的浏览器端直接选择文件夹,而不需要走这个绕一圈(
呜呜我一开始分析和判断的有问题,我删了重发了,您以我后面发的为准
就,弹窗本身确实没有什么危害,但麻烦的点在于这套文件操作API里还提供了ListImportDir和ReadImportFile这种可以实现任意文件读取的大雷。
There was a problem hiding this comment.
如果不是 localhost 的访问,或者不是 photino 而是本机 chromium 之类的,还是用像原先一样的浏览器端直接选择文件夹,而不需要走这个绕一圈(
对啊,现在就已经是这样的了,只有isPhotino才pickViaBackend,否则还是前端文件选择
MaiChartManager/MaiChartManager/Front/src/utils/pickDirectory.ts
Lines 30 to 51 in 2a32f30
There was a problem hiding this comment.
或者把”打开界面“的按钮改成拉起系统浏览器打开网页,而不是photino 我其实推荐这个
说真的,我最早的 Linux 想法就是纯后端,然后拉起 {google-chrome,chromium,microsoft-edge} --app=http://localhost。其实这样似乎是体验更好的,因为现在 webkit-git 甚至还有一些显示 bug,而 --app 的话,甚至不会有浏览器自带的 chrome(指地址栏、工具栏这些,不是谷歌浏览器)就和 Windows 上体验差不多。
但是我觉得可能不是所有人电脑上都有 chromium 系浏览器,而且后来调查发现了 Photino 这个东西,但是我也不知道 webkit-gtk 有这么多不兼容的东西,我就拿这个做了,后来才发现一堆不兼容的坑然后开始填
There was a problem hiding this comment.
或者把”打开界面“的按钮改成拉起系统浏览器打开网页,而不是photino 我其实推荐这个
说真的,我最早的 Linux 想法就是纯后端,然后拉起
{google-chrome,chromium,microsoft-edge} --app=http://localhost。其实这样似乎是体验更好的,因为现在 webkit-git 甚至还有一些显示 bug,而 --app 的话,甚至不会有浏览器自带的 chrome(指地址栏、工具栏这些,不是谷歌浏览器)就和 Windows 上体验差不多。但是我觉得可能不是所有人电脑上都有 chromium 系浏览器,而且后来调查发现了 Photino 这个东西,但是我也不知道 webkit-gtk 有这么多不兼容的东西,我就拿这个做了,后来才发现一堆不兼容的坑然后开始填
所以变动性比较小的改动是不是,本地模式还是photino不变,远程模式的话直接xdg-open一个http://,这样应该会调用系统默认浏览器。尽管不是所有人电脑上都有 chromium 系浏览器,但应该所有人电脑上都至少有个浏览器吧,而且大概率会是个支持window.showDirectoryPicker的浏览器
There was a problem hiding this comment.
而且大概率会是个支持
window.showDirectoryPicker的浏览器
似乎,支持这个的浏览器也就是 chromium 系的浏览器了
不过不支持的他们也有办法自行解决吧,毕竟用 Linux 了应该默认是有能力的了
涉及项目构建、前端逻辑、MP3兼容模式等,具体内容详见commit message。
Summary by Sourcery
改进跨平台行为,特别是针对 Linux/导出模式、音频解码和宿主检测,同时优化谱面导入体验和本地化。
新特性:
错误修复:
增强:
Original summary in English
Summary by Sourcery
Improve cross-platform behavior, especially around Linux/export modes, audio decoding, and host detection, while tightening chart import UX and localization.
New Features:
Bug Fixes:
Enhancements: