Conversation
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
Co-authored-by: Theo Browne <t3dotgg@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
github-actions
bot
added
size:XXL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| let subscribed = false; | ||
| const unsubServerConfigUpdated = onServerConfigUpdated((payload) => { | ||
| void queryClient.invalidateQueries({ queryKey: serverQueryKeys.config() }); | ||
| if (!subscribed) return; | ||
| const issue = payload.issues.find((entry) => entry.kind.startsWith("keybindings.")); | ||
| if (!issue) { | ||
| toastManager.add({ | ||
| type: "success", | ||
| title: "Keybindings updated", | ||
| description: "Keybindings configuration reloaded successfully.", | ||
| }); | ||
| return; | ||
| } | ||
| const themeIssue = payload.issues.find((entry) => entry.kind.startsWith("themes.")); | ||
| const keybindingIssue = payload.issues.find((entry) => entry.kind.startsWith("keybindings.")); | ||
| const themeUpdated = payload.updated.includes("themes"); | ||
| const keybindingsUpdated = payload.updated.includes("keybindings"); | ||
|
|
||
| toastManager.add({ | ||
| type: "warning", | ||
| title: "Invalid keybindings configuration", | ||
| description: issue.message, | ||
| actionProps: { | ||
| children: "Open keybindings.json", | ||
| onClick: () => { | ||
| void queryClient | ||
| .ensureQueryData(serverConfigQueryOptions()) | ||
| .then((config) => { | ||
| const editor = resolveAndPersistPreferredEditor(config.availableEditors); | ||
| if (!editor) { | ||
| throw new Error("No available editors found."); | ||
| } | ||
| return api.shell.openInEditor(config.keybindingsConfigPath, editor); | ||
| }) | ||
| .catch((error) => { | ||
| void syncThemeConfig().then((config) => { | ||
| if (!subscribed || !config) { | ||
| return; | ||
| } | ||
|
|
||
| if (themeUpdated) { | ||
| if (themeIssue) { |
There was a problem hiding this comment.
🟡 Medium routes/__root.tsx:277
The subscribed guard at line 286 is ineffective because onServerConfigUpdated invokes the listener synchronously during subscription, but the callback schedules syncThemeConfig().then(...) which runs as a microtask. By the time the .then() callback executes, line 365 has already set subscribed = true, so the guard !subscribed at line 286 is always false and duplicate toasts are not suppressed for replayed cached values. Consider adding a synchronous flag check before the async work, or restructuring so the guard is checked synchronously when the listener is first invoked.
let subscribed = false;
+ let skippedFirstReplay = false;
const unsubServerConfigUpdated = onServerConfigUpdated((payload) => {
+ if (!subscribed) {
+ skippedFirstReplay = true;
+ }
void queryClient.invalidateQueries({ queryKey: serverQueryKeys.config() });
const themeIssue = payload.issues.find((entry) => entry.kind.startsWith("themes."));
const keybindingIssue = payload.issues.find((entry) => entry.kind.startsWith("keybindings."));
const themeUpdated = payload.updated.includes("themes");
const keybindingsUpdated = payload.updated.includes("keybindings");
void syncThemeConfig().then((config) => {
- if (!subscribed || !config) {
+ if (skippedFirstReplay || !config) {
return;
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/routes/__root.tsx around lines 277-291:
The `subscribed` guard at line 286 is ineffective because `onServerConfigUpdated` invokes the listener synchronously during subscription, but the callback schedules `syncThemeConfig().then(...)` which runs as a microtask. By the time the `.then()` callback executes, line 365 has already set `subscribed = true`, so the guard `!subscribed` at line 286 is always false and duplicate toasts are not suppressed for replayed cached values. Consider adding a synchronous flag check before the async work, or restructuring so the guard is checked synchronously when the listener is first invoked.
Evidence trail:
apps/web/src/routes/__root.tsx lines 274-365 at REVIEWED_COMMIT (guard implementation and `subscribed = true` assignment); apps/web/src/wsNativeApi.ts lines 45-59 at REVIEWED_COMMIT (synchronous listener invocation with `listener(latestConfig)` at line 53); apps/web/src/components/KeybindingsToast.browser.tsx lines 354-366 (test comment confirming the intent that replayed cached value should NOT produce a toast)
2 participants
What Changed
themes.jsonfile in the user's.t3state area and applies them across the app via CSS variablesWhy
UI Changes
themes.jsonChecklist
Note
Add built-in and custom theme palettes with server-managed themes.json
ThemePalettesystem in themePalettes.ts with built-in palettes and support for custom palettes defined in a server-managedthemes.jsonfilethemes.json, exposing a snapshot and change stream to the WebSocket serverserverGetConfigresponses to includethemesConfigPathandcustomThemes, and adds anupdateddiscriminator array toserverConfigUpdatedpush payloadsserverConfigUpdatedpayloads now require anupdatedarray field; existing consumers must handle the new schema📊 Macroscope summarized 8275681. 21 files reviewed, 3 issues evaluated, 1 issue filtered, 1 comment posted
🗂️ Filtered Issues
apps/server/src/wsServer.ts — 0 comments posted, 1 evaluated, 1 filtered
serverConfigUpdatedpush notifications for keybindings changes (line 633) and themes changes (line 641) each only include their ownevent.issuesin theissuesfield. However, theserverGetConfighandler (line 905) returns combined issues:[...keybindingsConfig.issues, ...themesConfig.issues]. This means when a keybindings change push arrives, the client receives only keybinding issues and loses any theme issues (and vice versa). Since theissuesarray is flat and contains no discriminant to tell keybinding issues from theme issues, the client cannot correctly merge partial updates. Before this change there was only one source of issues so this inconsistency did not exist. [ Failed validation ]