Fix widget goals tests. - #6170
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The updated test file currently has cleanup that won’t run on failure (and includes unused/avoidable imports), which can still allow child-window leakage and potentially fail lint/typecheck.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes goal widget settings forms not rendering after load by ensuring the React render path tracks the widget loading state, and stabilizes goal widget e2e tests by preventing the child window from leaking into subsequent tests.
Changes:
- Updated
GenericGoalrender gating to also readw.state.isLoadingdirectly so loading completion triggers a re-render. - Added test cleanup to close the widget settings (child) window after the “change settings” test.
File summaries
| File | Description |
|---|---|
| app/components-react/widgets/GenericGoal.tsx | Adds an explicit reactive dependency on w.state.isLoading so settings UI renders after loading completes. |
| test/regular/widgets/goals.ts | Ensures the child window is closed after the settings test to avoid cross-test leakage. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
BundleMonFiles updated (1)
Unchanged files (3)
Total files change +207B 0% Final result: ✅ View report in BundleMon website ➡️ |
| </Menu> | ||
| <Form> | ||
| {w.hasLoadedSettings() && w.selectedTab === 'goal' && !hasGoal && ( | ||
| {!w.state.isLoading && w.hasLoadedSettings() && w.selectedTab === 'goal' && !hasGoal && ( |
There was a problem hiding this comment.
Confirmed this is happening, though it's not an issue in the actual prod flow since it's already loaded.
I don't think that we can't both get reactivity and TypeScript predicates to play nice, since the way it works would require mutating the object to use useMemo or useCallback and corrupt the underlying service object.
Let's just remove hasLoadedSettings and inline its check in all the widgets, I'd prefer that over using the sugar and then also having to re-write half of its logic outside the function every time.
Should just need to:
- Delete
hasLoadedSettingsfunction - Find target:
w.hasLoadedSettings()- Replace with:
!w.state.isLoading && w.settings
- Replace with:
- Find target:
this.hasLoadedSettings()- Replace with:
!this.state.isLoading && this.settings
- Replace with:
- Find target:
[w.settings, w.hasLoadedSettings]- Replace with:
[w.state.isLoading, w.settings]
- Replace with:
There was a problem hiding this comment.
🟡 Changes recommended
There are test/maintainability issues that should be addressed before merge (unused import likely failing lint/typecheck, misleading new comment referencing a removed API, and non-guaranteed test cleanup on failure paths).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
test/regular/widgets/goals.ts:5
useFormis imported but never used in this test file; unused imports tend to fail lint/typecheck and make it harder to spot real dependencies.
import { assertFormContains, fillForm, useForm } from '../../helpers/modules/forms';
test/regular/widgets/goals.ts:82
closeWindow('child')only runs on the happy path; if any step/assertion above throws, the child window can still leak into subsequent tests. Consider moving the cleanup into afinallyblock or an AVA teardown hook (still skipping macOS) so it runs even on failures.
if (platform() !== 'darwin') {
await closeWindow('child');
}
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
| return this.widgetState.data; | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the widget has loaded settings, and narrows the type of `this.settings` accordingly. | ||
| * | ||
| * NOTE: Since the type is narrowed via the `this` value, the static analysis will not work with | ||
| * object destructuring! Make sure to keep/use a reference to the module instance. | ||
| */ | ||
| hasLoadedSettings(): this is this & { | ||
| settings: TWidgetState['data']['settings']; | ||
| } { | ||
| return !!this.settings && !this.state.isLoading; | ||
| } | ||
|
|
||
| /** | ||
| * returns widget's settings from the store | ||
| */ |
Co-authored-by: Wes Rupert <wesrupert@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The goals test still has reliability issues (cleanup only on happy path) and includes an unused import that may fail lint/typecheck.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
app/components-react/widgets/GenericGoal.tsx:78
- The
!w.state.isLoading && w.settingsgating is intentionally redundant (sincew.settingsis already null while loading) and is easy to “simplify” later and regress the re-render behavior. Adding a short inline note here would help prevent accidental removal.
test/regular/widgets/goals.ts:5
useFormis imported but never used in this test file; this will typically fail lint/typecheck and adds noise to the import list.
import { assertFormContains, fillForm, useForm } from '../../helpers/modules/forms';
test/regular/widgets/goals.ts:82
- The child-window cleanup still only runs on the happy path; if any step above throws, the child window can leak into subsequent tests. Wrapping the settings interactions in a try/finally keeps cleanup reliable (while still skipping macOS where
closeWindowcrashes).
if (platform() !== 'darwin') {
await closeWindow('child');
}
app/components-react/widgets/common/useWidget.tsx:175
- The PR description says only
GenericGoal.tsxandtest/regular/widgets/goals.tschanged, but this PR also updates multiple widget components and removesWidgetModule.hasLoadedSettings(). Please update the PR description/title to reflect the actual scope so reviewers know to validate the broader widget settings rendering behavior.
/**
* returns widget's settings from the store
*/
get settings(): TWidgetState['data']['settings'] | null {
return !this.state.isLoading && this.widgetData.settings ? this.widgetData.settings : null;
}
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
Fix Goal Widgets Never Rendering Their Settings Form After Load
Issues
The Tip Goal, Follower Goal, Bit Goal failed consistently because of the Generic Goal form reactivity using a function instead of a getter for gating.
Fixes
GenericGoal.tsx:w.hasLoadedSettings() && ...gates also now check!w.state.isLoadingdirectly.state.isLoadingis part of the reactively-injectedstatesubmodule (genuinelyreactive: true), so reading it directly in the render registers it as a tracked dependency — its flip fromtruetofalsenow correctly triggers a re-render, after whichhasLoadedSettings()re-evaluates fresh and returns the right value. Left a comment explaining why the extra check is necessary, since the double-condition otherwise looks redundant.test/regular/widgets/goals.ts: addedcloseWindow('child')at the end of thechange settingstest (skipped on macOS viaplatform() !== 'darwin') so the widget settings window doesn't leak into the next test.Files changed:
app/components-react/widgets/GenericGoal.tsx,test/regular/widgets/goals.tsPerformance Implications
None. This only changes when a settings form re-renders on the client — no additional requests, state, or work on any path.