[Research]: Review and improve project configuration options #1937
+1,564
−16
Conversation
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
Contributor
Summary of ChangesHello @IsmaelMartinez, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a thorough research document that critically evaluates the current configuration system. It highlights existing problems with organization, documentation, and consistency, and proposes a structured, multi-phase approach to refactor the configuration into a more maintainable and discoverable nested format. The primary goal is to enhance the user experience and developer maintainability without immediate breaking changes. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces an excellent and thorough research document outlining a phased plan to refactor the application's configuration. The analysis is detailed, and the proposed migration strategy is sound, prioritizing backward compatibility and minimizing risk. My review includes a few minor suggestions to enhance the clarity and consistency of the new documentation.
docs-site/docs/development/research/configuration-organization-research.md
Outdated
Show resolved
Hide resolved
docs-site/docs/development/research/configuration-organization-research.md
Outdated
Show resolved
Hide resolved
docs-site/docs/development/research/configuration-organization-research.md
Outdated
Show resolved
Hide resolved
IsmaelMartinez
changed the title
Owner
Author
|
To pick this up after the next batch of changes, as we do have a few docs re-org in them. |
IsmaelMartinez
force-pushed
the
claude/investigate-config-improvements-011CUxCoL2TgC9MrQYTNBFCW
branch
from
4b73211 to
be5cbe6
Compare
- Document analysis of all 53 configuration options - Identify issues: scattered options, missing MQTT docs, deprecated options - Propose three-phase migration plan from flat to nested structure - Include detailed migration mapping and backward compatibility strategy - Add research document to Strategic Analysis section Key findings: - MQTT config exists but undocumented - Related options scattered (notifications: 9 opts, idle: 4 opts) - Naming inconsistencies (disable* vs *Enabled) - Recommend gradual migration with auto-conversion for v2.x See docs-site/docs/development/research/configuration-organization-research.md
- Sort Strategic Analysis section alphabetically in README.md - Correct option counts: Notifications & UI (8→10), Screen Sharing (6→7), System Integration (9→10) - Fix Authentication & SSO formatting: use nested lists instead of h4 headers - Fix migration table: appTitle note should be "Moved" not "Renamed" - Fix broken Docusaurus link: use GitHub URL for MQTT README instead of relative path Addresses feedback from Gemini and fixes Docusaurus build error.
MQTT documentation was added in PR #1939 after this research began, addressing one of the key findings. Updated the research document to reflect this progress: Changes: - Mark MQTT documentation as RESOLVED in Executive Summary - Update Key Finding #1 to show completion status - Mark Problem 1 as RESOLVED with solution details - Update Phase 1 changes to mark MQTT as completed - Update Phase 1 roadmap checklist to mark MQTT task done - Update success criteria to mark MQTT as completed - Reduce remaining Phase 1 effort estimate (4-6h → 2-4h) - Update conclusion with Phase 1 status - Add MQTT documentation PR #1939 to Related Issues The research remains valid - MQTT was one of several issues identified. Remaining work includes documentation reorganization and deprecated option migration guide.
IsmaelMartinez
force-pushed
the
claude/investigate-config-improvements-011CUxCoL2TgC9MrQYTNBFCW
branch
from
be5cbe6 to
3718256
Compare
Contributor
📦 PR Build Artifacts✅ Build successful! Download artifacts:
|
Remove deprecated configuration options and update research document Changes: - Remove contextIsolation option from app/config/index.js (lines 185-192) - Remove sandbox option from app/config/index.js (lines 388-395) - Both options were deprecated and never actually used in the code - Users with these in config.json will have them safely ignored (no breaking change) - Reduces total configuration options from 53 to 51 Research document updates: - Mark Problem 3 (deprecated options) as RESOLVED - Update executive summary: 53 → 51 options - Update inventory section to show options as removed - Update Key Finding #3 as resolved - Update Phase 1 deliverables to mark deprecated options as completed - Update Phase 1 roadmap checklist - Update success criteria - Update conclusion and next steps - Reduce remaining Phase 1 effort: 2-4h → 1-2h (MQTT done, deprecated options done) Phase 1 progress: 2 of 3 deliverables complete (MQTT ✅, deprecated options ✅) Remaining: Documentation reorganization (1-2 hours)
…ea approach Corrections based on feedback: 1. Update option count: 53 → 51 active options after deprecation removal 2. Update yargs config block size: ~484 lines → ~395 lines 3. Remove Phase 1 items that are no longer applicable: - "Document deprecated options" (we removed them instead) - "Add migration guide section" (not needed for Phase 1) 4. Simplify Phase 1 to just documentation reorganization 5. Update Phase 2 to reflect incremental area-by-area migration strategy: - Can align with feature work (e.g., notifications refactoring) - Use yargs' built-in deprecated field for user warnings - Auto-migration logic converts old → new structure - Test each area thoroughly before moving to next 6. Update examples to show yargs deprecated field usage This approach is more practical and can be done incrementally as features are refactored, rather than all at once.
Update Phase 2 implementation strategy based on feedback: 1. Dedicated Migration Module (app/config/migration.js): - Cleaner architecture - separate migration logic from main config - Easier to maintain and test - Area-specific migration functions (notifications, window, auth) - Clear documentation with JSDoc 2. Auto-Fix Strategy (not just warnings): - Automatically converts old keys to new structure in-memory - Users get informed via console but don't need to do anything - Old config.json remains untouched (no write operations) - Works transparently every app launch 3. Cross-Platform Compatibility: - Works with Vanilla, Snap, and Flatpak installations - In-memory only migration avoids sandbox write permission issues - Config files read from platform-specific locations but migration is universal - Users can migrate to new format at their own pace Benefits: - Zero user action required - migration happens automatically - No breaking changes - old configs keep working - Clean separation of concerns - Testable migration logic - Works across all platforms without special handling
Extend migration strategy to include optional disk-write capability based on user feedback: New Feature: Auto-Fix Config File - Shows one-time dialog prompting user to update their config.json - Three options: "Update Now", "Keep Current", "Ask Me Later" - Creates backup (config.json.backup) before any changes - Removes old keys and adds new nested structure - Uses marker file (.config-auto-fix-offered) to avoid nagging Benefits: 1. Faster deprecation timeline - users migrate voluntarily 2. Reduced long-term maintenance burden 3. User has full control - explicit opt-in required 4. Safe - automatic backup before changes 5. Works cross-platform - user config dirs are writable in snap/flatpak Accelerated Deprecation Timeline: - v2.x (Phase 2a): Nested options + migration + auto-fix prompt - v2.x+1 (Phase 2b): Deprecate old keys (yargs warnings) - v2.x+2 (Phase 2c): After 6mo, old keys migration-only - v3.0 (Phase 3): Remove old keys completely This allows us to clean up old options much sooner while still providing a smooth migration path for users who prefer manual migration.
Removed "Keep Current" option from migration prompt based on feedback: Changes: - Dialog now has only two buttons: "Update Now" and "Ask Me Later" - Removed "Keep Current" option that would permanently decline migration - Users who want to keep in-memory migration can click "Ask Me Later" repeatedly - Simpler UX that gently encourages migration without being pushy - Updated benefits list to reflect new two-option approach Rationale: - Two options are clearer and less overwhelming - Encourages gradual migration without forcing it - Users who truly don't want to migrate will just keep postponing - No permanent "decline" option means more users will eventually migrate
Updated Implementation Roadmap and Risk Assessment sections to ensure consistency with the automatic migration approach: - Phase 3: Changed from "Breaking Changes" to "Automatic Migration" - Automatic disk-write in v3.0 without user prompt - Migration logic kept indefinitely (only removed in v4.0+ when usage drops) - Updated risk assessment from MEDIUM-HIGH to LOW-MEDIUM - Updated success criteria to focus on automatic migration - Zero breaking changes - all existing configs continue working This completes the comprehensive zero-breakage strategy that ensures no user configurations will ever break, while still achieving a clean config structure for new installations.
|
Owner
Author
|
blocked by #1959 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Key findings:
See docs-site/docs/development/research/configuration-organization-research.md