-
Notifications
You must be signed in to change notification settings - Fork 0
fix(frontend): resolve SSE warnings, add ETA, and prevent DOM errors in full_techniques mode #272
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -61,12 +61,13 @@ export const ProgressTopBar: React.FC<ProgressTopBarProps> = ({ | |||||
| </div> | ||||||
|
|
||||||
| <div className="flex items-center gap-4 text-sm"> | ||||||
| {enrichmentMessage && ( | ||||||
| <div className="flex items-center gap-1.5 px-3 py-1 rounded-full bg-amber-500/10 border border-amber-500/20 text-amber-700 text-xs font-medium"> | ||||||
| <Loader2 size={12} className="animate-spin" /> | ||||||
| <span className="truncate max-w-[150px]">{enrichmentMessage}</span> | ||||||
| </div> | ||||||
| )} | ||||||
| <div | ||||||
| className={`flex items-center gap-1.5 px-3 py-1 rounded-full bg-amber-500/10 border border-amber-500/20 text-amber-700 text-xs font-medium transition-opacity duration-300 ${enrichmentMessage ? 'opacity-100' : 'opacity-0 pointer-events-none'}`} | ||||||
| style={{ visibility: enrichmentMessage ? 'visible' : 'hidden' }} | ||||||
| > | ||||||
| <Loader2 size={12} className="animate-spin" /> | ||||||
| <span className="truncate max-w-[150px]">{enrichmentMessage || 'Loading...'}</span> | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fallback text
Suggested change
|
||||||
| </div> | ||||||
| <div className="hidden md:block text-gray-600 font-medium"> | ||||||
| <span className="text-[#722F37] font-bold">{completedTechniques}</span> | ||||||
| <span className="text-gray-400 mx-1">/</span> | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,6 +89,8 @@ const INITIAL_CATEGORIES: Record<string, CategoryStatus> = Object.entries(CATEGO | |
| {} | ||
| ); | ||
|
|
||
| const ESTIMATED_TOTAL_SECONDS = 600; | ||
|
|
||
| const initialState: FullTechniquesStreamState = { | ||
| connectionStatus: 'connecting', | ||
| retryCount: 0, | ||
|
|
@@ -103,6 +105,8 @@ const initialState: FullTechniquesStreamState = { | |
| isComplete: false, | ||
| tokensUsed: 0, | ||
| costUsd: 0, | ||
| startedAt: new Date().toISOString(), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
To fix this, Since a code suggestion cannot span multiple parts of the file, here is an example of how you could implement this: // 1. Create a function to generate the initial state
const createInitialState = (): FullTechniquesStreamState => ({
// ... other initial state properties from your existing initialState object
startedAt: new Date().toISOString(),
etaSeconds: ESTIMATED_TOTAL_SECONDS,
});
// 2. Use this function for lazy initialization in the hook
const [state, dispatch] = useReducer(reducer, undefined, createInitialState);
// 3. Use the function in your reducer's RESET case
case 'RESET':
return createInitialState(); |
||
| etaSeconds: ESTIMATED_TOTAL_SECONDS, | ||
| enrichmentPhase: 'idle', | ||
| enrichmentMessage: null, | ||
| enrichmentStatus: { | ||
|
|
@@ -116,7 +120,8 @@ type Action = | |
| | { type: 'CONNECTION_CHANGE'; status: FullTechniquesStreamState['connectionStatus']; retryCount?: number } | ||
| | { type: 'EVENT_RECEIVED'; event: SSEEvent } | ||
| | { type: 'ERROR'; error: string } | ||
| | { type: 'RESET' }; | ||
| | { type: 'RESET' } | ||
| | { type: 'UPDATE_ETA'; elapsedSeconds: number }; | ||
|
|
||
| function reducer(state: FullTechniquesStreamState, action: Action): FullTechniquesStreamState { | ||
| switch (action.type) { | ||
|
|
@@ -334,6 +339,23 @@ function reducer(state: FullTechniquesStreamState, action: Action): FullTechniqu | |
| return newState; | ||
| } | ||
|
|
||
| case 'UPDATE_ETA': { | ||
| if (state.isComplete || state.completedTechniques === 0) { | ||
| return state; | ||
| } | ||
|
|
||
| const { elapsedSeconds } = action; | ||
| const completionRatio = state.completedTechniques / state.totalTechniques; | ||
|
|
||
| if (completionRatio > 0 && completionRatio < 1) { | ||
| const estimatedTotalTime = elapsedSeconds / completionRatio; | ||
| const remainingSeconds = Math.max(0, Math.ceil(estimatedTotalTime - elapsedSeconds)); | ||
| return { ...state, etaSeconds: remainingSeconds }; | ||
| } | ||
|
|
||
| return state; | ||
| } | ||
|
|
||
| default: | ||
| return state; | ||
| } | ||
|
|
@@ -427,5 +449,18 @@ export function useFullTechniquesStream(evaluationId: string | null) { | |
| }; | ||
| }, [evaluationId, state.isComplete, token]); | ||
|
|
||
| useEffect(() => { | ||
| if (!evaluationId || state.isComplete) return; | ||
|
|
||
| const startTime = state.startedAt ? new Date(state.startedAt).getTime() : Date.now(); | ||
|
|
||
| const interval = setInterval(() => { | ||
| const elapsedSeconds = Math.floor((Date.now() - startTime) / 1000); | ||
| dispatch({ type: 'UPDATE_ETA', elapsedSeconds }); | ||
| }, 5000); | ||
|
|
||
| return () => clearInterval(interval); | ||
| }, [evaluationId, state.isComplete, state.startedAt]); | ||
|
|
||
| return state; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition to check the current stage is repeated and a bit long. For better readability and maintainability, you can use an array and the
includesmethod. This also makes it easier to add more stages in the future.