-
Notifications
You must be signed in to change notification settings - Fork 279
Improve theme server to have both palette and theme in json file #5865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| host: 'localhost', | ||
| port: '8008', | ||
| overrideThemeFile: '/Users/username/Documents/overrideTheme.json' | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Hardcoded developer path in default configuration
The overrideThemeFile default value contains a hardcoded developer-specific path /Users/username/Documents/overrideTheme.json. This path won't exist on other developers' machines or in CI/production environments, causing file read/write operations in themeServer.ts to fail with file-not-found errors. The default appears to be placeholder/example text rather than a valid default path.
|
|
||
| const envConfig = asEnvConfig(envJSON) | ||
| const { overrideThemeFile } = envConfig.THEME_SERVER | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Config setting lost when env.json is rewritten
The overrideThemeFile configuration property is extracted from envConfig.THEME_SERVER at line 33, but when env.json is written back at lines 53-57, envJSON.THEME_SERVER is overwritten with only host and port. This causes any user-configured overrideThemeFile value in env.json to be permanently lost after the first server run. Subsequent server starts will fall back to the default value instead of the user's configured path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole concept of the themeParser is awful! This could only exist in an age of AI where we can produce terrible ideas faster than we can understand or maintain them. But... you probably knew that already. Actually, this is fine as long as we finish the light theme project before anybody makes any major refactors to the edgeDark.ts file. The theme server will get the job done for now, and by the time this fragile approach inevitably breaks, we will hopefully be past the point where we even care. So yes, this PR is approved, because this isn't a part of the "real" app and it's better to have working tools than not.
If we were to do this "properly", some ideas would be to either import the edgeDark.js file as code and let Node.js handle the parsing, or flip things around and generate edgeDark.js from some templates. There is no reason to do either of these things, though, since we have a working approach for the time being.
|
/rebase |
25b4b22 to
773a9b5
Compare
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have:
Note
Adds a theme parser and updates the theme server to read/write a palette+theme JSON format using a configurable override file path.
scripts/themeParser.ts: ParsesedgeDark.tsto extractpalette, identifies runtime props, converts values to palette refs, merges runtime theme into{ palette, theme }, and resolves palette refs for serving.scripts/themeServer.ts:mergeThemeandparseOverrideThemeto handle the new{ palette, theme }format.THEME_SERVER.overrideThemeFile; logs JSON and serves resolved flat theme viaGET /theme.THEME_SOURCE_PATHand minor typing/paths cleanup.src/envConfig.ts: AddsTHEME_SERVER.overrideThemeFileoption with defaults.eslint.config.mjs: Removesscripts/themeServer.tsfrom the warnings exceptions list.Written by Cursor Bugbot for commit 773a9b5. This will update automatically on new commits. Configure here.