feat: expand preference editor and add settings reference#362
Draft
meganetaaan wants to merge 1 commit intodev/v1.0from
Draft
feat: expand preference editor and add settings reference#362meganetaaan wants to merge 1 commit intodev/v1.0from
meganetaaan wants to merge 1 commit intodev/v1.0from
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces comprehensive preference documentation (English and Japanese) detailing preference keys and usage patterns. It extends firmware preference keys for driver and TTS domains, adds defensive aliasing in driver preference loading, and significantly refactors the web preference editor UI with field validation, conditional visibility, and toast notifications. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Collaborator
Author
|
@coderabbitai review plz |
Contributor
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
web/preference/index.html (1)
126-130: Handle disconnect failures explicitly.Line 127 can reject and currently leaves an unhandled async error path.
💡 Proposed fix
const handleDisconnectButtonClick = async () => { - await client.disconnect() - updateView() - showToast('Disconnected', 'info') + try { + await client.disconnect() + showToast('Disconnected', 'info') + } catch (error) { + console.error(error) + showToast(`Disconnect failed: ${String(error)}`, 'error') + } finally { + updateView() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/preference/index.html` around lines 126 - 130, The async handler handleDisconnectButtonClick calls client.disconnect() without catching rejections; wrap the await in a try/catch so failures are handled: inside handleDisconnectButtonClick, try awaiting client.disconnect(), then call updateView() and showToast('Disconnected','info') on success; in the catch block log the error (e.g., console.error or a logger) and call showToast with an error message (include error.message) so the UI reflects the failure and no unhandled rejection occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@firmware/docs/setting-preferences-web_ja.md`:
- Around line 39-40: Replace the wording that implies "消去" with phrasing that
clearly indicates "上書き": update the sentence
"既存値を消したい場合は、消去対象を明示的に設定したうえで再起動確認してください。" to explicitly use "上書き" (for example
"既存値を上書きしたい場合は、上書き対象を明示的に設定したうえで再起動を確認してください。") and ensure the preceding line
still states that empty fields are not sent so the intent to overwrite (上書き) is
unambiguous.
In `@web/preference/index.html`:
- Around line 60-63: The UI toggles visibility via setVisible(selector, visible)
but hidden fields remain enabled and are still collected/submitted; update
setVisible to also disable/enable all form controls inside the selector (e.g.,
input, select, textarea, button) when toggling visibility so hidden groups
cannot be submitted, and ensure the form-collection logic skips elements that
are disabled or whose container has the 'hidden' class (i.e., when gathering
values check element.disabled or container.classList.contains('hidden') so
token/driver fields are never sent while hidden).
- Around line 151-153: The finally block currently always sets
submitButton.disabled = false which can re-enable the UI even if Bluetooth
disconnected; change the flow so the button is only re-enabled when the BLE
client is actually connected (e.g., check the connection flag or call
client.connected/isConnected before setting submitButton.disabled = false)
and/or move the re-enable into the success path after client.send; additionally
hook the BLE disconnect event (or client.ondisconnect) to explicitly disable
submitButton when the connection drops so submit-button state stays in sync with
the real connection state (refer to submitButton, client.send, and the finally
block).
---
Nitpick comments:
In `@web/preference/index.html`:
- Around line 126-130: The async handler handleDisconnectButtonClick calls
client.disconnect() without catching rejections; wrap the await in a try/catch
so failures are handled: inside handleDisconnectButtonClick, try awaiting
client.disconnect(), then call updateView() and showToast('Disconnected','info')
on success; in the catch block log the error (e.g., console.error or a logger)
and call showToast with an error message (include error.message) so the UI
reflects the failure and no unhandled rejection occurs.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
firmware/docs/flashing-firmware.mdfirmware/docs/flashing-firmware_ja.mdfirmware/docs/preferences.mdfirmware/docs/preferences_ja.mdfirmware/docs/setting-preferences-web.mdfirmware/docs/setting-preferences-web_ja.mdfirmware/stackchan/main.tsfirmware/stackchan/utilities/consts.tsweb/preference/index.htmlweb/preference/preference.css
Comment on lines
+39
to
+40
| > Web 設定アプリでは、空欄の項目は送信されません。 | ||
| > 既存値を消したい場合は、消去対象を明示的に設定したうえで再起動確認してください。 |
Contributor
There was a problem hiding this comment.
「消去」ではなく「上書き」に寄せた表現にしたほうが正確です。
現在の文言だと値の削除が可能に読めるため、上書き意図を明確化するのが安全です。
✏️ Proposed wording fix
-> 既存値を消したい場合は、消去対象を明示的に設定したうえで再起動確認してください。
+> 既存値を上書きしたい場合は、置き換え値を明示的に設定し、再起動後に反映を確認してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/docs/setting-preferences-web_ja.md` around lines 39 - 40, Replace
the wording that implies "消去" with phrasing that clearly indicates "上書き": update
the sentence "既存値を消したい場合は、消去対象を明示的に設定したうえで再起動確認してください。" to explicitly use "上書き"
(for example "既存値を上書きしたい場合は、上書き対象を明示的に設定したうえで再起動を確認してください。") and ensure the
preceding line still states that empty fields are not sent so the intent to
overwrite (上書き) is unambiguous.
Comment on lines
+60
to
+63
| const setVisible = (selector, visible) => { | ||
| document.querySelectorAll(selector).forEach((element) => { | ||
| element.classList.toggle('hidden', !visible) | ||
| }) |
Contributor
There was a problem hiding this comment.
Hidden conditional fields are still being submitted.
Line 60 only toggles visibility, but Line 93 still collects all non-empty inputs. This can resend stale values from hidden TTS/driver groups (including token fields) unintentionally.
💡 Proposed fix
const setVisible = (selector, visible) => {
document.querySelectorAll(selector).forEach((element) => {
element.classList.toggle('hidden', !visible)
+ element.querySelectorAll('input, select, textarea').forEach((control) => {
+ control.disabled = !visible
+ })
})
}
const collectPayload = () => {
const payload = {}
for (const field of FIELD_DEFINITIONS) {
const element = document.getElementById(field.id)
- if (element == null) {
+ if (element == null || element.disabled) {
continue
}Also applies to: 91-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/preference/index.html` around lines 60 - 63, The UI toggles visibility
via setVisible(selector, visible) but hidden fields remain enabled and are still
collected/submitted; update setVisible to also disable/enable all form controls
inside the selector (e.g., input, select, textarea, button) when toggling
visibility so hidden groups cannot be submitted, and ensure the form-collection
logic skips elements that are disabled or whose container has the 'hidden' class
(i.e., when gathering values check element.disabled or
container.classList.contains('hidden') so token/driver fields are never sent
while hidden).
Comment on lines
+151
to
153
| } finally { | ||
| submitButton.disabled = false | ||
| } |
Contributor
There was a problem hiding this comment.
Keep submit-button state synced with BLE connection state.
Line 152 always enables the button. If disconnect happens during client.send, the UI can end up enabled while disconnected.
💡 Proposed fix
} catch (error) {
console.error(error)
showToast(`Submit failed: ${String(error)}`, 'error')
} finally {
- submitButton.disabled = false
+ updateView()
}📝 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.
Suggested change
| } finally { | |
| submitButton.disabled = false | |
| } | |
| } catch (error) { | |
| console.error(error) | |
| showToast(`Submit failed: ${String(error)}`, 'error') | |
| } finally { | |
| updateView() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/preference/index.html` around lines 151 - 153, The finally block
currently always sets submitButton.disabled = false which can re-enable the UI
even if Bluetooth disconnected; change the flow so the button is only re-enabled
when the BLE client is actually connected (e.g., check the connection flag or
call client.connected/isConnected before setting submitButton.disabled = false)
and/or move the re-enable into the success path after client.send; additionally
hook the BLE disconnect event (or client.ondisconnect) to explicitly disable
submitButton when the connection drops so submit-button state stays in sync with
the real connection state (refer to submitButton, client.send, and the finally
block).
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
Documentation
New Features
Bug Fixes