Skip to content

fix: patch help_improve bug#1455

Merged
Wendong-Fan merged 2 commits intomainfrom
patch-help_improve
Mar 5, 2026
Merged

fix: patch help_improve bug#1455
Wendong-Fan merged 2 commits intomainfrom
patch-help_improve

Conversation

@a7m-1st
Copy link
Copy Markdown
Collaborator

@a7m-1st a7m-1st commented Mar 5, 2026

Related Issue

Closes #1454

Description

  • for now only looks for specific fields before checking:
const REQUIRED_FIELDS = [
  'take_screenshot',
  'access_local_software',
  'access_your_address',
  'password_storage',
];

Testing Evidence (REQUIRED)

  • I have included human-verified testing evidence in this PR.
  • This PR includes frontend/UI changes, and I attached screenshot(s) or screen recording(s).
  • No frontend/UI changes in this PR.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Contribution Guidelines Acknowledgement

@a7m-1st a7m-1st self-assigned this Mar 5, 2026
Copy link
Copy Markdown
Contributor

@eureka0928 eureka0928 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! The core logic change (checking only the 4 required fields instead of all response keys) is correct and solves the immediate bug.

A few observations:

1. Root cause is on the backend
The real issue is that GET /api/user/privacy returns the raw pricacy_setting JSON column (server/app/controller/user/user_controller.py:80), which leaks any extra key ever written to it. If the backend filtered through UserPrivacySettings:

return UserPrivacySettings(**model.pricacy_setting).model_dump()

then unknown fields like help_improve would be stripped at the source, and the original frontend code would work fine. Worth considering a backend-side fix too — the frontend fix alone means any future field added to the stored JSON could re-introduce this bug.

2. Hardcoded field list is fragile
If the UserPrivacySettings model gains a new required field later, someone needs to remember to update this frontend list too. Consider either:

  • Fixing on the backend (preferred, see above)
  • Or at minimum, adding a comment linking to the backend model so future devs know to keep them in sync

3. Tailwind class reordering adds review noise
The diff has ~15 lines of pure Tailwind class reordering (alphabetizing). While cosmetically fine, mixing formatting changes with a bug fix makes the PR harder to review and increases merge conflict risk with other open branches. Ideally these would be in a separate commit or PR.

Overall the approach is sound — just needs the formatting noise separated and testing evidence added. The backend-side fix would be the more robust long-term solution.

Copy link
Copy Markdown
Contributor

@Wendong-Fan Wendong-Fan left a comment

Choose a reason for hiding this comment

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

thanks @a7m-1st , based on @eureka928 's comment, added enhance PR #1456, feel free to check

Co-authored-by: a7m-1st <Ahmed.jimi.awelkeir500@gmail.com>
@Wendong-Fan Wendong-Fan merged commit 737bd1e into main Mar 5, 2026
6 checks passed
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.

[BUG] New privacy field (help_improve) causes UI bug

3 participants