Stop makeFunctionReference being defined inside components#17
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Review Summary by QodoExtract Convex function references outside components and add hardening tests
WalkthroughsDescription• Extract Convex function references to module-level constants • Add comprehensive test for component-scoped Convex references • Improve type safety in mutation handling and error cases • Add accessibility and utility improvements across components Diagramflowchart LR
A["Component-scoped<br/>makeFunctionReference calls"] -->|Extract to<br/>module level| B["Top-level<br/>constants"]
B -->|Use in<br/>components| C["useQuery/useMutation<br/>hooks"]
D["Type safety<br/>improvements"] -->|Add to<br/>test suite| E["typeHardeningGuard<br/>test"]
E -->|Detect violations| F["Component-scoped<br/>Convex refs"]
File Changes1. apps/web/src/app/typeHardeningGuard.test.ts
|
Code Review by Qodo
1. Full-tree scan in test
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the web frontend to avoid defining Convex makeFunctionReference calls inside component bodies by extracting them into module-scope constants, adds a guard test to prevent regressions, and includes a couple of small type-safety/accessibility improvements.
Changes:
- Extracted/renamed Convex query/action/mutation references to top-level constants across multiple pages/components.
- Added a repository-wide guard test to detect component-scoped Convex ref factories in
apps/web/src. - Improved exhaustiveness checking for outbound message UI helpers and added an
aria-labelfor the outbound banner dismiss button.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/convex/tests/setupTestAdminFallback.ts | Tightens typing around the ConvexClient mutation fallback used in tests. |
| apps/web/src/components/WorkspaceSelector.tsx | Moves the create-workspace mutation reference to module scope. |
| apps/web/src/components/SuggestionsPanel.tsx | Extracts settings/suggestions refs to constants and updates hook usage. |
| apps/web/src/components/AudienceRuleBuilder.tsx | Extracts segment/audience preview query refs to module scope. |
| apps/web/src/components/AppSidebar.tsx | Extracts sidebar queries to module-scope constants. |
| apps/web/src/app/typeHardeningGuard.test.ts | Adds a guard that scans apps/web/src for component-scoped Convex ref factories. |
| apps/web/src/app/settings/AuditLogViewer.tsx | Extracts audit-related query/mutation refs to module scope. |
| apps/web/src/app/outbound/page.tsx | Extracts outbound list/create/pause/etc refs to module scope. |
| apps/web/src/app/outbound/outboundMessageUi.tsx | Adds assertNever for exhaustive handling of status/type unions. |
| apps/web/src/app/outbound/[id]/OutboundTriggerPanel.tsx | Adds integer parsing helper and updates trigger numeric inputs. |
| apps/web/src/app/outbound/[id]/OutboundPreviewPanel.tsx | Adds type="button" and aria-label to dismiss control in preview. |
| apps/web/src/app/onboarding/page.tsx | Extracts hosted onboarding refs and related types/utilities to module scope. |
| apps/web/src/app/inbox/InboxConversationListPane.tsx | Extracts the visitor presence query ref to module scope. |
| apps/web/src/app/campaigns/email/[id]/page.tsx | Extracts campaign queries/mutations refs to module scope for the editor page. |
Comments suppressed due to low confidence (1)
apps/web/src/app/outbound/[id]/OutboundTriggerPanel.tsx:87
scrollPercentnow uses??andparseOptionalInteger, so values like0(or >100) can be kept in state even though the input is constrained to 1–100. Consider clamping/validating the parsed number before callingonChange(e.g., ignore values outside 1..100 or coerce to bounds).
value={value.scrollPercent ?? 50}
onChange={(e) =>
onChange({ ...value, scrollPercent: parseOptionalInteger(e.target.value) })
}
min={1}
max={100}
/>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
71
| value={value.delaySeconds ?? 5} | ||
| onChange={(e) => | ||
| onChange({ ...value, delaySeconds: Number.parseInt(e.target.value, 10) }) | ||
| onChange({ ...value, delaySeconds: parseOptionalInteger(e.target.value) }) | ||
| } | ||
| min={1} |
There was a problem hiding this comment.
Switching from || to ?? means a value of 0 will now be treated as valid and shown in the number input, even though the input has min={1}. Consider normalizing/clamping parsed values (e.g., treat <= 0 as undefined/default) to avoid persisting an invalid delaySeconds state.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
djanogly
added a commit
that referenced
this pull request
Mar 11, 2026
* complete proposal reduce convex ref escape hatches * Stop makeFunctionReference being defined inside components (#17) Broader fix for calling makeFunctionReference inside components * complete proposal reduce convex ref escape hatches * harden types
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
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.
This pull request refactors several frontend files to standardize the naming and usage of Convex function references, improves code clarity, and introduces a few utility functions and accessibility enhancements. The main changes include extracting and renaming Convex function references to constants, reordering code for readability, and adding utility functions for type safety and error handling.
The most important changes are:
Refactoring Convex function references:
makeFunctionReferencecalls to top-level constants with consistent uppercase naming across multiple files, replacing inline definitions in components. This enhances code clarity and maintainability. (apps/web/src/app/campaigns/email/[id]/page.tsxL66-R107, [1] [2] [3] [4] [5] [6] [7] [8]Code organization and readability:
formatTimestamp,formatClientType) and type definitions out of the main component body inonboarding/page.tsxfor better organization. [1] [2]Utility and type safety improvements:
parseOptionalIntegerutility function to safely parse integer values and handle invalid input inOutboundTriggerPanel. (apps/web/src/app/outbound/[id]/OutboundTriggerPanel.tsxR12-R16, apps/web/src/app/outbound/[id]/OutboundTriggerPanel.tsxL62-R69, apps/web/src/app/outbound/[id]/OutboundTriggerPanel.tsxL76-R83)assertNeverfunction to enforce exhaustive checking in switch statements for outbound message status and type, improving type safety. [1] [2] [3]Accessibility:
aria-labelto the dismiss button in the outbound banner preview for better accessibility. (apps/web/src/app/outbound/[id]/OutboundPreviewPanel.tsxL97-R103)Let me know if you'd like to discuss any specific change in more detail!