replace deriveAsync and deriveAsyncIfDefined with runed resource#1695
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes remove the custom asynchronous debouncing utilities ( Changes
Assessment against linked issues
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
UI unit Tests10 tests 10 ✅ 0s ⏱️ Results for commit c3e641f. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/src/lib/forms/UserTypeahead.svelte (2)
42-44: Add explicit generic toresourcefor safer typings
runedwill inferany[]for the resource unless a generic is supplied. Supplying the expected element type improves IntelliSense and prevents silent type regressions.-const _typeaheadResults = resource(() => trigger, (value) => typeaheadSearch(value), {initialValue: [], debounce: debounceMs}); +const _typeaheadResults = resource<UserTypeaheadResult[]>( + () => trigger, + (value) => typeaheadSearch(value), + { initialValue: [], debounce: debounceMs } +);
132-133: ConfirmonInputpayload is always a string
PlainInputsometimes forwards the originalInputEvent. If that happens,value ?? ''will yield'[object InputEvent]'rather than the empty string, re-triggering the search with garbage.
Please ensurePlainInput’sonInputalways supplies astring, or narrow the type here:-onInput={(value) => trigger = value ?? ''} +onInput={(v) => trigger = typeof v === 'string' ? v : ''}frontend/src/routes/(authenticated)/project/create/+page.ts (1)
109-113: Guard clause still allows 1-character language codes
_getProjectsByLangCodeAndOrgonly short-circuits for lengths other than 2 or 3, meaning a single-characterlangCodeproceeds to the network call.
That is unlikely to be valid and wastes a request. Consider extending the guard:-if (input.langCode.length !== 3 && input.langCode.length !== 2) return []; +if (input.langCode.length !== 2 && input.langCode.length !== 3) return [];(also trims whitespace to avoid
" "being considered 1 char).frontend/src/routes/(authenticated)/project/create/+page.svelte (2)
105-112: Avoid network hit while user is still typing very short codesThe availability check fires as soon as
code.length >= 1.
You already know codes shorter than4fail validation, so you can skip the fetch entirely and save server load:- if (!browser || !code || !user.canCreateProjects || code === defaultCode) return true; + if (!browser || !code || code.length < 4 || !user.canCreateProjects || code === defaultCode) + return true;
186-193: Watcher mutates form on every keystroke – consider batching
watch(() => code, …)writes back into the form on each character, marking it dirty and potentially triggering expensive validations.
Debounce or batch the update to improve responsiveness for very long inputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/lib/forms/UserTypeahead.svelte(4 hunks)frontend/src/lib/util/time.test.ts(0 hunks)frontend/src/lib/util/time.ts(0 hunks)frontend/src/routes/(authenticated)/project/create/+page.svelte(6 hunks)frontend/src/routes/(authenticated)/project/create/+page.ts(2 hunks)
💤 Files with no reviewable changes (2)
- frontend/src/lib/util/time.test.ts
- frontend/src/lib/util/time.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build API / publish-api
- GitHub Check: Build UI / publish-ui
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: check-and-lint
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
frontend/src/lib/forms/UserTypeahead.svelte (1)
48-50: Race-condition risk when callingonSelectedUserChange
selectUsersetsselectedUserand immediately invokes the callback with that reactive reference.
If the parent mutatesselectedUserinside the callback, the assignment order may cause surprising results.
Consider passing an immutable clone, or at minimum dereferencing.value/.currentbefore emitting.frontend/src/routes/(authenticated)/project/create/+page.ts (1)
128-130: Mirror the min-length check from the Zod schema
projectName.length < 3aligns with the resource debounce, but the form actually requiresmin(1).
If you tighten the guard to 3, make sure the schema/UX communicates this to the user to avoid silent “nothing happens” states.
Runed resource requires an async fetcher function, so even when we can resolve without async we need to return a resolved Promise.
rmunn
left a comment
rmunn
left a comment
There was a problem hiding this comment.
LGTM. There was a reason why we used deriveAsyncIfDerived at one point, but I think we prevented the possibility of undefined getting returned from those GQL queries, so although we're not replicating the if (value) { ... } behavior of deriveAsyncIfDerived, that's probably okay.
3 participants
closes #980
I also did a bit of refactoring to simplify our related project handling