-
Notifications
You must be signed in to change notification settings - Fork 3.2k
v0.2.8: fix + feat + improvement #621
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
Conversation
* fix(sharing): fixed folders not appearing when sharing workflows * cleanup * fixed error case
… throughout (#620) * use cascade deletion * fix lint --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@vikhyaths-air.lan>
* chore: use t3-env as source of truth * chore: update mock env for failing tests
* feat(logs): enhanced logging system with cleanup and theme fixes - Implement enhanced logging cleanup with S3 archival and retention policies - Fix error propagation in trace spans for manual executions - Add theme-aware styling for frozen canvas modal - Integrate enhanced logging system across all execution pathways - Add comprehensive trace span processing and iteration navigation - Fix boolean parameter types in enhanced logs API * add warning for old logs * fix lint * added cost for streaming outputs * fix overflow issue * fix lint * fix selection on closing sidebar * tooltips z index increase --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@vikhyaths-air.lan> Co-authored-by: Waleed Latif <walif6@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
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.
PR Summary
Major update introducing a new tokenization system and improving workspace permissions, with significant security and logging enhancements across the application.
- Added comprehensive tokenization system in
apps/sim/lib/tokenization/for cost tracking and token estimation across different LLM providers - Enhanced workspace security by requiring admin permissions for destructive operations in
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/control-bar/control-bar.tsx - Implemented workspace-based folder permissions in
apps/sim/app/api/folders/replacing user-based access control - Improved database connection handling in
apps/sim/db/index.tswith conservative pool settings to prevent connection exhaustion - Replaced legacy logging with EnhancedLoggingSession across execution endpoints for better observability
52 files reviewed, 39 comments
Edit PR Review Bot Settings | Greptile
|
|
||
| const IS_DEV = process.env.NODE_ENV === 'development' | ||
| const IS_DEV = env.NODE_ENV === 'development' |
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.
style: Consider making IS_DEV a memoized value since env.NODE_ENV won't change during runtime
| SOCKET_PORT: z.number().optional(), | ||
| PORT: z.number().optional(), |
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.
style: Missing schema validation constraints. Consider adding min/max values for port numbers to prevent invalid configurations.
| SOCKET_PORT: z.number().optional(), | |
| PORT: z.number().optional(), | |
| SOCKET_PORT: z.number().min(1).max(65535).optional(), | |
| PORT: z.number().min(1).max(65535).optional(), |
| vi.mock('../env', () => ({ | ||
| env: { | ||
| FREE_TIER_COST_LIMIT: 5, | ||
| PRO_TIER_COST_LIMIT: 20, | ||
| TEAM_TIER_COST_LIMIT: 40, | ||
| ENTERPRISE_TIER_COST_LIMIT: 200, | ||
| }, | ||
| })) |
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.
style: Consider mocking values as strings to match actual env var types.
| edges: sampleWorkflowState.edges || [], | ||
| loops: sampleWorkflowState.loops || {}, | ||
| parallels: sampleWorkflowState.parallels || {}, | ||
| parallels: {}, |
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.
style: Consider keeping the optional fallback for parallels consistent with loops and edges to maintain pattern consistency
| const executionId = uuidv4() | ||
| await persistExecutionLogs(workflowId, executionId, enrichedResult, 'chat') | ||
| logger.debug(`Persisted logs for deployed chat: ${executionId}`) | ||
| logger.debug(`Generated execution ID for deployed chat: ${executionId}`) |
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.
logic: using duplicate executionId - already generated on line 256
| const executionId = uuidv4() | |
| await persistExecutionLogs(workflowId, executionId, enrichedResult, 'chat') | |
| logger.debug(`Persisted logs for deployed chat: ${executionId}`) | |
| logger.debug(`Generated execution ID for deployed chat: ${executionId}`) | |
| logger.debug(`Using execution ID for deployed chat: ${executionId}`) |
| export function estimateInputTokens( | ||
| systemPrompt?: string, | ||
| context?: string, | ||
| messages?: Array<{ role: string; content: string }>, | ||
| providerId?: string | ||
| ): TokenEstimate { |
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.
style: Consider grouping optional parameters into a config object for better maintainability as more providers or options are added.
| export function estimateInputTokens( | |
| systemPrompt?: string, | |
| context?: string, | |
| messages?: Array<{ role: string; content: string }>, | |
| providerId?: string | |
| ): TokenEstimate { | |
| type InputTokensConfig = { | |
| systemPrompt?: string | |
| context?: string | |
| messages?: Array<{ role: string; content: string }> | |
| providerId?: string | |
| } | |
| export function estimateInputTokens(config: InputTokensConfig): TokenEstimate { |
| switch (effectiveProviderId) { | ||
| case 'openai': | ||
| case 'azure-openai': | ||
| estimatedTokens = estimateOpenAITokens(text) | ||
| break | ||
| case 'anthropic': | ||
| estimatedTokens = estimateAnthropicTokens(text) | ||
| break | ||
| case 'google': | ||
| estimatedTokens = estimateGoogleTokens(text) | ||
| break | ||
| default: | ||
| estimatedTokens = estimateGenericTokens(text, config.avgCharsPerToken) | ||
| } |
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.
style: As the provider list grows, consider extracting each provider's estimation logic into separate functions in a dedicated config file.
|
|
||
| if (messages) { | ||
| for (const message of messages) { | ||
| totalText += `${message.role}: ${message.content}\n` |
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.
logic: Concatenating role and content with a colon may cause issues if the role or content contains colons. Consider using a more robust separator or different format.
| if (!text || text.length < MIN_TEXT_LENGTH_FOR_ESTIMATION) { | ||
| return { | ||
| count: 0, | ||
| confidence: 'high', | ||
| provider: providerId || 'unknown', | ||
| method: 'fallback', | ||
| } | ||
| } |
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.
logic: Early return uses 'high' confidence but returns 'fallback' method, which seems inconsistent with the confidence level.
| if (!text || text.length < MIN_TEXT_LENGTH_FOR_ESTIMATION) { | |
| return { | |
| count: 0, | |
| confidence: 'high', | |
| provider: providerId || 'unknown', | |
| method: 'fallback', | |
| } | |
| } | |
| if (!text || text.length < MIN_TEXT_LENGTH_FOR_ESTIMATION) { | |
| return { | |
| count: 0, | |
| confidence: 'low', | |
| provider: providerId || 'unknown', | |
| method: 'fallback', | |
| } | |
| } |
| const traceSpans = (executionResult.logs || []).map((blockLog: any, index: number) => { | ||
| let output = blockLog.output | ||
| if (!blockLog.success && blockLog.error) { | ||
| output = { | ||
| error: blockLog.error, | ||
| success: false, | ||
| ...(blockLog.output || {}), | ||
| } | ||
| } | ||
|
|
||
| // Persist logs for this execution using the standard 'webhook' trigger type | ||
| await persistExecutionLogs(foundWorkflow.id, executionId, enrichedResult, 'webhook') | ||
| return { | ||
| id: blockLog.blockId, | ||
| name: `Block ${blockLog.blockName || blockLog.blockType} (${blockLog.blockType || 'unknown'})`, | ||
| type: blockLog.blockType || 'unknown', | ||
| duration: blockLog.durationMs || 0, | ||
| startTime: blockLog.startedAt, | ||
| endTime: blockLog.endedAt || blockLog.startedAt, | ||
| status: blockLog.success ? 'success' : 'error', | ||
| blockId: blockLog.blockId, | ||
| input: blockLog.input, | ||
| output: output, | ||
| tokens: blockLog.output?.tokens?.total || 0, | ||
| relativeStartMs: index * 100, | ||
| children: [], | ||
| toolCalls: (blockLog as any).toolCalls || [], | ||
| } | ||
| }) |
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.
style: Consider extracting this trace span mapping logic to a separate function for better maintainability.
|
✅ No security or compliance issues detected. Reviewed everything up to 0bf9ce0. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. Reply to this PR with |
* fix(sharing): fixed folders not appearing when sharing workflows (simstudioai#616) * fix(sharing): fixed folders not appearing when sharing workflows * cleanup * fixed error case * fix(deletions): folder deletions were hanging + use cascade deletions throughout (simstudioai#620) * use cascade deletion * fix lint --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@vikhyaths-air.lan> * fix(envvars): t3-env standardization (simstudioai#606) * chore: use t3-env as source of truth * chore: update mock env for failing tests * feat(enhanced logs): integration + log visualizer canvas (simstudioai#618) * feat(logs): enhanced logging system with cleanup and theme fixes - Implement enhanced logging cleanup with S3 archival and retention policies - Fix error propagation in trace spans for manual executions - Add theme-aware styling for frozen canvas modal - Integrate enhanced logging system across all execution pathways - Add comprehensive trace span processing and iteration navigation - Fix boolean parameter types in enhanced logs API * add warning for old logs * fix lint * added cost for streaming outputs * fix overflow issue * fix lint * fix selection on closing sidebar * tooltips z index increase --------- Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@vikhyaths-air.lan> Co-authored-by: Waleed Latif <walif6@gmail.com> --------- Co-authored-by: Waleed Latif <walif6@gmail.com> Co-authored-by: Vikhyath Mondreti <vikhyathmondreti@vikhyaths-air.lan> Co-authored-by: Aditya Tripathi <aditya@climactic.co>
Description
Type of change
Checklist:
bun run test)Security Considerations: