-
-
Notifications
You must be signed in to change notification settings - Fork 778
#2223 - elderberry #2363
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
#2223 - elderberry #2363
Conversation
|
WalkthroughA middleware function named Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx}📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (8)📓 Common learnings
📚 Learning: applies to **/trigger/**/*.{ts,tsx,js,jsx} : when logging in tasks, use the `logger` api (`logger.de...
Applied to files:
📚 Learning: applies to **/*.{ts,tsx} : always prefer using isomorphic code like fetch, readablestream, etc. inst...
Applied to files:
📚 Learning: applies to **/trigger/**/*.{ts,tsx,js,jsx} : when using realtime features, use the `runs.subscribeto...
Applied to files:
📚 Learning: applies to **/trigger/**/*.{ts,tsx,js,jsx} : when using metadata in tasks, use the `metadata` api as...
Applied to files:
📚 Learning: applies to **/trigger/**/*.{ts,tsx,js,jsx} : you must use `@trigger.dev/sdk/v3` when writing trigger...
Applied to files:
📚 Learning: applies to **/trigger/**/*.{ts,tsx,js,jsx} : when using idempotency, use the `idempotencykeys` api a...
Applied to files:
📚 Learning: in apps/webapp/app/services/runsrepository.server.ts, the in-memory status filtering after fetching ...
Applied to files:
🔇 Additional comments (7)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
special pr for eric
Adding a small SWR middleware utility to log if a fetcher is present (indicating a potential global fetcher), and wiring it into our realtime hooks' useSWR calls to validate the assumption without changing behavior. I'll implement the utility and update
useRealtime.ts
accordingly.Ran tool
Ran tool
Ran tool
packages/react-hooks
for our internal realtime hooks and how they use SWR. Found that our realtime hooks intentionally use SWR as a local store by passingnull
as the fetcher (so they shouldn’t use any globalSWRConfig
fetcher).SWRConfig
fetcher might still impact us:null
, inadvertently falling back to the global fetcher.useId()
).What I’m doing now:
useSWR
calls to detect if a fetcher (likely from globalSWRConfig
) is present. This adheres to your “add logs to validate assumptions before fixing” direction, without changing behavior.Edits made:
logIfFetcherPresent
middleware:packages/react-hooks/src/utils/swrMiddleware.ts
packages/react-hooks/src/hooks/useRealtime.ts
(forrun
,streams
,error
,complete
,runs
)These logs only fire in development and won’t break anything. If a global fetcher is being used by our realtime hooks, you’ll see warnings like:
[trigger.dev][realtime:run] Detected a fetcher for SWR key. This hook is intended to manage its own data; an inherited global SWR fetcher may cause unintended requests.
Proposed most likely root causes to validate with these logs:
useSWR
calls that should passnull
accidentally omit it (or regressions in future changes).<SWRConfig value={{ fetcher: ... }}>
, and SWR ends up injecting that fetcher into our hooks if we’d ever forget thenull
guard.Next steps (after we confirm via logs):
Harden prevention:
null
and disables revalidation options for our realtime store calls.['@trigger.dev/realtime', idKey, 'run']
) to reduce any chance of collision with consumer keys.Keep current behavior for SWR-based fetching hooks (like
useRun
) that intentionally provide a fetcher.Changes:
logIfFetcherPresent
middleware inpackages/react-hooks/src/utils/swrMiddleware.ts
.packages/react-hooks/src/hooks/useRealtime.ts
to every internaluseSWR
store usage to log if a fetcher is present (potential global fetcher injection).Impact:
SWRConfig
fetchers are ever invoked by our realtime hooks.