-
Notifications
You must be signed in to change notification settings - Fork 8.2k
⚡️ Speed up function get_cache_service by 1,090% in PR #11129 (aka-merge-1.7.1)
#11132
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
base: main
Are you sure you want to change the base?
⚡️ Speed up function get_cache_service by 1,090% in PR #11129 (aka-merge-1.7.1)
#11132
Conversation
* Revert "Revert "docs: update component documentation links to individual pages"" This reverts commit 0bc27d6. * [autofix.ci] apply automated fixes * llm-selector-renamed * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * Apply suggestions from code review * [autofix.ci] apply automated fixes * Apply suggestions from code review * [autofix.ci] apply automated fixes * rebuild-component-index * update-component-index * [autofix.ci] apply automated fixes * build-index * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…10586) * fix: resolved merge conflict * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * fix: create a new message to avoid mutating shared instances * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * fix: resolved merge conflict * [autofix.ci] apply automated fixes * fix: resolved merge conflict * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * fix: added a check for using exisiting message object * fix: remove unwanted import * fix: resolve merge conflict * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * fix: add None checks to prevent errors * fix: resolve merge conflict * [autofix.ci] apply automated fixes * fix: backend unit test * fix: resolve merge conflict * [autofix.ci] apply automated fixes * fix: ruff styling errors * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* feat: optimize dropdown filtering and output resolution misc: remove commented out code feat: add refresh button and sort flows by updated_at date from most to least recent ruff (flow.py imports) improve fn contracts in runflow and improve flow id retrieval logic based on graph exec context add dynamic outputs and optimize db lookups add flow cache and db query for getting a single flow by id or name cache run outputs and add refresh context to build config misc misc use ids for flow retrieval misc fix missing flow_id bug add unit and integration tests add input field flag to persist hidden fields at runtime move unit tests and change input and output display names chore: update component index fix: fix tool mode when flow has multiple inputs by dynamically creating resolvers chore: update component index ruff (run_flow and tests) add resolvers to outputs map for non tool mode runtime fix tests (current flow excluded in db fetch) mypy (helpers/flow.py) chore: update component index remove unused code and clean up comments fix: persist user messages in chat-based flows via session injection chore: update component index empty string fallback for sessionid in chat.py chore: update component index chore: update component index cache invalidation with timestamps misc add cache invalidation chore: update component index chore: update comp idx ruff (run_flow.py) change session_id input type to MessageTextInput chore: update component index chore: update component index chore: update component index chore: update component index sync starter projects with main chore: update component index chore: update component index chore: update component index remove dead code + impl coderabbit suggestions chore: update component index chore: update component index clear options metadata before updating chore: update component index sync starter projects with main sync starter projects with main default param val (list flows) * chore: update component index * add integration tests * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) --------- Co-authored-by: Cristhian Zanforlin <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
upgrade altk:
…ls (#10806) * use existing event loop instead of recreating when calling mcp tools * component index * [autofix.ci] apply automated fixes * starter projects * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* removed unnecessary buttons on the flows page * added the asChild prop and hid button so they are not accessible by tabbing * added tab index to ensure that buttons as not selectable using the tab * made sure that accessibility is possible one bulk selection is enabled * made sure that accessibility is possible one bulk selection is enabled * Fix: added testcases and refactor * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes --------- Co-authored-by: Olayinka Adelakun <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* remove console warnings * [autofix.ci] apply automated fixes --------- Co-authored-by: Olayinka Adelakun <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* fix: mask value to hide null field being returned * [autofix.ci] apply automated fixes * fix: added testcase and updated functionality --------- Co-authored-by: Olayinka Adelakun <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Carlos Coelho <[email protected]> Co-authored-by: Olayinka Adelakun <[email protected]>
#10827) Fix: Allow refresh list button to stay stagnant while zoom (Safari) (#10777) * remove sticky as it was causing the refresh list to float on safari * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes --------- Co-authored-by: Olayinka Adelakun <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This reverts commit 423419e.
* fix: Ollama model list fails to load in Agent and Ollama components * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* fix: made sure the tab is visible * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * Fix: added typing * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * fix: added testcases * fix: added handleOnValue change function and created a helper file --------- Co-authored-by: Olayinka Adelakun <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Olayinka Adelakun <[email protected]> Co-authored-by: Carlos Coelho <[email protected]>
Co-authored-by: Sami Marreed <[email protected]>
Remove DataFrameToToolsetComponent and related tests Deleted the DataFrameToToolsetComponent implementation, its import/registration in the processing module, and all associated unit tests. This cleans up unused code and test files related to converting DataFrame rows into toolset actions.
fix: Proper parsing of GCP credentials JSON (#10828) * fix: Proper parsing of GCP credentials JSON * Update save_file.py * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * Update test_save_file_component.py * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * Fix GCP issues * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * Update test_save_file_component.py * Update save_file.py * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * Update save_file.py * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * Update save_file.py * Fix ruff errors * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* fix: Suppress SIGSEGV errors on startup (#10849) * fix: Suppress SIGSEGV errors * Update test_cli.py * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * Update News Aggregator.json Co-Authored-By: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* fix: Don't fail if doc column is missing * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * Surface warning message to the UI * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * Update test_docling_utils.py * [autofix.ci] apply automated fixes * Update test_docling_utils.py --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
) * fix: Support Batch Run with watsonX (#10848) * fix: Support Batch Run with watsonX * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * Update batch_run.py * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
* fix: Image upload for Gemini/Anthropic (#10867) * Fix image upload for Gemini/Anthropic and ChatOutput session_id preservation * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * fix ruff erros * [autofix.ci] apply automated fixes * resolve conflicts * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * build component index * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) --------- Co-Authored-By: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Himavarsha <[email protected]>
fix: Clean up the default startup logging (#10842) * fix: Clean up the default startup logging * [autofix.ci] apply automated fixes * Update manager.py * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * Update test_security_cors.py * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Himavarsha <[email protected]>
fix lfx serve asyncio event loop bug
* fix: Advanced mode in read file component (#11041) * add a proper file path * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * Update file.py Co-Authored-By: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * build component index * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * build component index * [autofix.ci] apply automated fixes * Fix incorrect use of .tempdir Co-Authored-By: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Eric Hare <[email protected]> * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Eric Hare <[email protected]>
fix: validate flow file_save path is in a valid location (#11039) * Validate flow file save path is in a valid location * clean up logic * fix tests * comments * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * fix backslash vuln * [autofix.ci] apply automated fixes * add storage service param to function in agentic utils * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * Ruff errors * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * Resolve path in setup * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * [autofix.ci] apply automated fixes (attempt 3/3) * comp index update * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Eric Hare <[email protected]>
…s to processingCreate List and flow_controlsPass for consistency and clarity in decision flow tests
* refactor: Use customization to get api base urls (#10871) * fixed counts * use customization to get api base urls --------- Co-authored-by: Deon Sanchez <[email protected]> * refactor: add code sample customizations (#10884) * add code sample customizations * import cleanup * embedded widget generator --------- Co-authored-by: Deon Sanchez <[email protected]>
* chore(release.yml): update release_lfx input description and make it optional to improve clarity feat(release.yml): add ensure-lfx-published job to automate LFX version check and publishing process to PyPI * chore: clean up release workflow comment out unneeded cross platform test and move steps around to match the already existing pattern --------- Co-authored-by: cristhianzl <[email protected]>
* chore(release.yml): update release_lfx input description and make it optional to improve clarity feat(release.yml): add ensure-lfx-published job to automate LFX version check and publishing process to PyPI * chore: clean up release workflow comment out unneeded cross platform test and move steps around to match the already existing pattern * chore: remove test-lfx-cross-platform remove test-lfx-cross-platform * chore: address some of co-pilots comments address some of co-pilots comments --------- Co-authored-by: cristhianzl <[email protected]>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
… 0.2.1 (#11108) * chore: update version to 0.2.1 in pyproject.toml * bump version to 1.7.1 in package.json * chore: bump version to 0.7.1 and update lfx dependency to 0.2.1 in pyproject.toml * chore: bump versions for langflow, langflow-base, and lfx in pyproject.toml and uv.lock * regenarate lock based on release branch * new package-lock fixed * fix: regenerate package-lock.json with missing nested dependencies Adds [email protected] (for rehype-mathjax) and [email protected] (for tailwindcss) that were missing from the lock file causing npm ci to fail. --------- Co-authored-by: cristhianzl <[email protected]>
…odes instead of Ctrl/Meta+click for better reliability in Playwright with ReactFlow
… rename tests to ensure stability test(general-bugs-reset-flow-run.spec.ts): add wait time to improve reliability of component build checks
The optimization caches the `CacheServiceFactory` instance using function attributes, eliminating redundant object creation on every call to `get_cache_service()`. **Key changes:** - **Factory Caching**: Instead of creating a new `CacheServiceFactory()` instance on every call, the optimization uses `hasattr()` to check if a cached factory exists on the function object itself (`get_cache_service._factory`). The factory is only created once and reused for subsequent calls. - **Lazy Import**: The expensive import of `CacheServiceFactory` is moved inside the conditional block, so it only executes once when the factory is first created. **Why this leads to speedup:** - **Object Creation Overhead**: Python object instantiation has overhead - constructor calls, memory allocation, and initialization. The original code creates a new `CacheServiceFactory` object 34 times (per the profiler), while the optimized version creates it only once. - **Import Cost Reduction**: Module imports in Python can be expensive, especially if they trigger cascading imports or module initialization code. Moving the import inside the conditional reduces this from 34 executions to 1. - **Function Call Elimination**: The optimized version avoids repeated `CacheServiceFactory()` constructor calls after the first execution. **Performance impact:** The line profiler shows the optimized version reduces total time for `get_cache_service()` from 78.7ms to 76.5ms, with most calls now hitting the cached path. The 1090% speedup suggests this function is called frequently in a hot path, making the per-call savings compound significantly. **Test case effectiveness:** The optimization works particularly well for test cases that make repeated calls to `get_cache_service()`, such as the singleton tests and large-scale tests that verify cache operations multiple times. The caching ensures consistent factory reuse across all these calls.
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## aka-merge-1.7.1 #11132 +/- ##
===================================================
- Coverage 33.22% 33.17% -0.05%
===================================================
Files 1392 1392
Lines 65996 65998 +2
Branches 9767 9767
===================================================
- Hits 21924 21895 -29
- Misses 42949 42981 +32
+ Partials 1123 1122 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Changes SummaryThis PR merges version 1.7.1 changes and includes a performance optimization for Type: mixed Components Affected: Backend service dependencies (deps.py), Message/monitor API endpoints, API utilities (remove_api_keys), LFX CLI and settings, Frontend MCP server hooks, Embed modal component, CI/CD workflows, Starter project templates Architecture Impact
Risk Areas: Authorization change in message endpoints - now filters by user's flows, could affect existing API consumers, Async session.add() handling in memory.py - checking for coroutines on synchronous method seems unusual, Performance optimization caches factory on function object - could persist state unexpectedly in certain testing scenarios, Database path resolution change in lfx settings - fallback behavior when langflow isn't importable Suggestions
Full review in progress... | Powered by diffray |
| </div> | ||
| </BaseModal.Header> | ||
| <BaseModal.Content className=""> | ||
| <div className="relative flex h-full w-full"> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={copyToClipboard} | ||
| data-testid="btn-copy-code" | ||
| className="!hover:bg-foreground group absolute right-2 top-2" | ||
| > | ||
| {isCopied ? ( | ||
| <IconComponent | ||
| name="Check" | ||
| className="h-5 w-5 text-muted-foreground" | ||
| /> | ||
| ) : ( | ||
| <IconComponent | ||
| name="Copy" | ||
| className="!h-5 !w-5 text-muted-foreground" | ||
| /> | ||
| )} | ||
| </Button> | ||
| <SyntaxHighlighter | ||
| showLineNumbers={true} | ||
| wrapLongLines={true} | ||
| language="html" | ||
| style={isDark ? oneDark : oneLight} | ||
| className="!mt-0 h-full w-full overflow-scroll !rounded-b-md border border-border text-left !custom-scroll" | ||
| > | ||
| {embedCode} | ||
| </SyntaxHighlighter> | ||
| </div> | ||
| <> | ||
| <CustomAPIGenerator isOpen={open} isEmbedded={true} /> | ||
| <div className="relative flex h-full w-full"> | ||
| <Button |
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.
🟡 MEDIUM - Promise without proper error handling
Category: quality
Description:
The navigator.clipboard.writeText() promise has no rejection handler at line 49-55, errors are silently ignored.
Suggestion:
Add proper error handling with .catch() that logs the error and optionally shows user feedback.
Confidence: 75%
Rule: ts_handle_async_operations_with_proper_erro
| if order_by: | ||
| col = getattr(MessageTable, order_by).asc() | ||
| stmt = stmt.order_by(col) | ||
| order_col = getattr(MessageTable, order_by).asc() |
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.
🟡 MEDIUM - Dynamic attribute access without validation
Category: bug
Description:
getattr(MessageTable, order_by) could raise AttributeError if order_by contains invalid attribute name. User-supplied parameter not validated.
Suggestion:
Add validation to ensure order_by is a valid MessageTable attribute before calling getattr, or use a whitelist of allowed values.
Confidence: 80%
Rule: bug_null_pointer
| </div> | ||
| </BaseModal.Header> | ||
| <BaseModal.Content className=""> | ||
| <div className="relative flex h-full w-full"> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={copyToClipboard} | ||
| data-testid="btn-copy-code" | ||
| className="!hover:bg-foreground group absolute right-2 top-2" | ||
| > | ||
| {isCopied ? ( | ||
| <IconComponent | ||
| name="Check" | ||
| className="h-5 w-5 text-muted-foreground" | ||
| /> | ||
| ) : ( | ||
| <IconComponent | ||
| name="Copy" | ||
| className="!h-5 !w-5 text-muted-foreground" | ||
| /> | ||
| )} | ||
| </Button> | ||
| <SyntaxHighlighter | ||
| showLineNumbers={true} | ||
| wrapLongLines={true} | ||
| language="html" | ||
| style={isDark ? oneDark : oneLight} | ||
| className="!mt-0 h-full w-full overflow-scroll !rounded-b-md border border-border text-left !custom-scroll" | ||
| > | ||
| {embedCode} | ||
| </SyntaxHighlighter> | ||
| </div> | ||
| <> | ||
| <CustomAPIGenerator isOpen={open} isEmbedded={true} /> |
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.
🟡 MEDIUM - Clipboard API error not fully handled
Category: bug
Description:
Check for clipboard existence is insufficient - writeText can still fail due to permissions or insecure context
Suggestion:
Wrap navigator.clipboard.writeText in try-catch to handle potential runtime errors beyond existence checking.
Confidence: 70%
Rule: bug_missing_try_catch
| ) | ||
|
|
||
|
|
||
| def get_starter_projects_path() -> Path: |
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.
🟡 MEDIUM - Duplicate utility function across test files
Category: consistency
Description:
get_starter_projects_path function duplicated in 4 test files with slightly different implementations
Suggestion:
Extract to shared test utility module like tests/utils/project_helpers.py and import in all test files.
Confidence: 85%
Rule: cons_duplicate_utility_function
| base_url_to_check, tool_model_enabled=tool_model_enabled | ||
| ) | ||
| else: | ||
| build_config["model_name"]["options"] = [] |
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.
🟡 MEDIUM - Docstring describes wrong filtering logic
Category: docs
Description:
Docstring says function filters models that 'do not have embedding capability' but implementation filters FOR models with 'completion' capability
Suggestion:
Update docstring to accurately describe that function filters for models with 'completion' capability (and optionally 'tools').
Confidence: 85%
Rule: py_docstring_capability_claim_false
| def get_cache_service() -> Union[CacheService, AsyncBaseCacheService]: # noqa: UP007 | ||
| """Retrieves the cache service from the service manager. | ||
| Returns: | ||
| The cache service instance. | ||
| """ | ||
| from langflow.services.cache.factory import CacheServiceFactory | ||
| if not hasattr(get_cache_service, "_factory"): | ||
| from langflow.services.cache.factory import CacheServiceFactory | ||
|
|
||
| return get_service(ServiceType.CACHE_SERVICE, CacheServiceFactory()) | ||
| get_cache_service._factory = CacheServiceFactory() | ||
|
|
||
| return get_service(ServiceType.CACHE_SERVICE, get_cache_service._factory) |
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.
🟡 MEDIUM - Function attribute used for caching
Category: quality
Description:
The function get_cache_service uses a mutable function attribute _factory for caching. This is not a standard Python pattern and could lead to unexpected behavior in multi-threaded environments.
Suggestion:
Consider using @lru_cache decorator or a proper singleton pattern instead of function attributes for caching
Confidence: 65%
Rule: python_class_attribute_mutable
| </div> | ||
| </BaseModal.Header> | ||
| <BaseModal.Content className=""> | ||
| <div className="relative flex h-full w-full"> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={copyToClipboard} | ||
| data-testid="btn-copy-code" | ||
| className="!hover:bg-foreground group absolute right-2 top-2" | ||
| > | ||
| {isCopied ? ( | ||
| <IconComponent | ||
| name="Check" | ||
| className="h-5 w-5 text-muted-foreground" | ||
| /> | ||
| ) : ( | ||
| <IconComponent | ||
| name="Copy" | ||
| className="!h-5 !w-5 text-muted-foreground" | ||
| /> | ||
| )} | ||
| </Button> | ||
| <SyntaxHighlighter | ||
| showLineNumbers={true} | ||
| wrapLongLines={true} | ||
| language="html" | ||
| style={isDark ? oneDark : oneLight} | ||
| className="!mt-0 h-full w-full overflow-scroll !rounded-b-md border border-border text-left !custom-scroll" | ||
| > | ||
| {embedCode} | ||
| </SyntaxHighlighter> | ||
| </div> | ||
| <> |
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.
🟡 MEDIUM - setTimeout missing cleanup in component
Category: bug
Description:
A setTimeout is used to reset the isCopied state after 2 seconds, but there's no cleanup mechanism if the component unmounts before the timeout completes.
Suggestion:
Store the timeout ID in a ref and clear it in a useEffect cleanup function, or use a custom hook that handles cleanup automatically.
Confidence: 80%
Rule: ts_clear_timers_on_teardown_unmount
| if order_by: | ||
| col = getattr(MessageTable, order_by).asc() | ||
| stmt = stmt.order_by(col) | ||
| order_col = getattr(MessageTable, order_by).asc() |
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.
🟠 HIGH - Unsafe dynamic attribute access allows arbitrary column access
Category: security
Description:
The order_by parameter is used directly with getattr(MessageTable, order_by) without validation. An attacker can specify any attribute name, potentially accessing internal methods or causing AttributeError exceptions.
Suggestion:
Implement a whitelist validation for the order_by parameter. Only allow specific, approved column names like 'timestamp', 'sender', 'sender_name', 'session_id', 'created_at'.
Confidence: 90%
Rule: py_add_input_validation_for_critical_parame
| ).toBeTruthy(); | ||
| } | ||
|
|
||
| // Copy configuration |
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.
🔵 LOW - Unused variable '_sseUrl'
Category: quality
Description:
Variable '_sseUrl' is extracted from regex match but never used. The underscore prefix suggests intentional non-use, but it appears to be incomplete code.
Suggestion:
If this variable is not needed, remove the assignment. If it's intended for future validation, add a comment or use it.
Confidence: 80%
Rule: quality_unused_variable
| import { CustomAPIGenerator } from "@/customization/components/custom-api-generator"; | ||
| import { useDarkStore } from "@/stores/darkStore"; | ||
| import IconComponent from "../../components/common/genericIconComponent"; | ||
| import { Button } from "../../components/ui/button"; |
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.
🟡 MEDIUM - Using empty object type {} instead of proper type
Category: quality
Description:
The 'tweaksBuildedObject' prop is typed as '{}' (empty object type), which provides minimal type safety.
Suggestion:
Define a proper interface for tweaksBuildedObject, or use 'Record<string, unknown>' for dynamic key-value objects.
Confidence: 75%
Rule: ts_object_type_vs_record
Review Summary
Validated 71 issues: 43 kept, 28 filtered Issues Found: 43See 12 individual line comment(s) for details. 📊 31 unique issue type(s) across 43 location(s) 📋 Full issue list (click to expand)🟠 HIGH - Console.error in production code - redundant logging (3 occurrences)Category: quality 📍 View all locations
Rule: 🟠 HIGH - Docstring returns section documents incorrect typeCategory: docs File: Description: The docstring for get_messages states it returns 'List[Data]' but the function signature returns 'list[Message]'. Same issue exists in aget_messages at line 111-112. Suggestion: Update the Returns section to document 'list[Message]' instead of 'List[Data]' to match the actual function return type. Confidence: 95% Rule: 🟠 HIGH - Missing None check before dictionary accessCategory: bug File: Description: In the remove_api_keys function, the code accesses node.get('data').get('node') without checking if node.get('data') returns None. This can cause an AttributeError at runtime. Suggestion: Add proper None checks: Confidence: 95% Rule: 🟠 HIGH - Unsafe dynamic attribute access allows arbitrary column accessCategory: security File: Description: The Suggestion: Implement a whitelist validation for the Confidence: 90% Rule: 🟠 HIGH - Function returns 'any' typeCategory: quality File: Description: The function 'scapeJSONParse' returns 'any', disabling type safety for all code that uses this function's result. Suggestion: Use 'unknown' instead: 'export function scapeJSONParse(json: string): unknown' and require callers to validate/assert types. Confidence: 95% Rule: 🟠 HIGH - Test without assertions on actual behaviorCategory: quality File: Description: Test test_is_storage_path_format only tests Python's built-in Path behavior, not the component's logic. Suggestion: Replace with actual component method calls and assertions on component behavior. Confidence: 85% Rule: 🟠 HIGH - Test without meaningful assertionsCategory: quality File: Description: Test ends with clicking a button but doesn't verify any expected outcome or result from the action. Suggestion: Add assertions to verify the expected behavior after clicking the send button, such as waiting for a response message or checking UI state. Confidence: 90% Rule: 🟠 HIGH - Type assertion without validation on error object (2 occurrences)Category: bug 📍 View all locations
Rule: 🟠 HIGH - Non-null assertion without validationCategory: bug File: Description: Accessing 'targetNode.data.node!.template[field]' with non-null assertion. While targetNode existence is checked at line 91, node.data.node may still be null. Suggestion: Add proper null checks: 'if (!targetNode.data.node?.template?.[field]) return;' Confidence: 75% Rule: 🟠 HIGH - Empty collection edge case not handled (2 occurrences)Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Unused variable 'count' (2 occurrences)Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Promise without proper error handlingCategory: quality File: Description: The navigator.clipboard.writeText() promise has no rejection handler at line 49-55, errors are silently ignored. Suggestion: Add proper error handling with .catch() that logs the error and optionally shows user feedback. Confidence: 75% Rule: 🟡 MEDIUM - Promise with empty catch handler silently swallows errors (2 occurrences)Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Dynamic attribute access without validation (2 occurrences)Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Clipboard API error not fully handledCategory: bug File: Description: Check for clipboard existence is insufficient - writeText can still fail due to permissions or insecure context Suggestion: Wrap navigator.clipboard.writeText in try-catch to handle potential runtime errors beyond existence checking. Confidence: 70% Rule: 🟡 MEDIUM - Dictionary key access without existence checkCategory: bug File: Description: Accessing models[self.JSON_MODELS_KEY] without checking if key exists could raise KeyError if API response is malformed Suggestion: Add check: if self.JSON_MODELS_KEY not in models: return [] before the loop, or use models.get(self.JSON_MODELS_KEY, []) Confidence: 75% Rule: 🟡 MEDIUM - Duplicate utility function across test filesCategory: consistency File: Description: get_starter_projects_path function duplicated in 4 test files with slightly different implementations Suggestion: Extract to shared test utility module like tests/utils/project_helpers.py and import in all test files. Confidence: 85% Rule: 🟡 MEDIUM - Docstring describes wrong filtering logicCategory: docs File: Description: Docstring says function filters models that 'do not have embedding capability' but implementation filters FOR models with 'completion' capability Suggestion: Update docstring to accurately describe that function filters for models with 'completion' capability (and optionally 'tools'). Confidence: 85% Rule: 🟡 MEDIUM - E2E test with real file I/O in unit test directoryCategory: quality File: Description: This test file performs real file I/O operations and end-to-end browser automation (Playwright) but is located in the unit test directory (core/unit/). E2E tests should be separated from unit tests. Suggestion: Move this test to the e2e/ or integration/ directory. Add an @e2e or @integration tag. Confidence: 85% Rule: 🟡 MEDIUM - Broad exception handling masks specific errorsCategory: quality File: Description: Multiple endpoints catch generic Exception without specifying exception types, making error diagnosis difficult and potentially hiding unexpected issues. Suggestion: Catch specific exception types (ValueError, KeyError, etc.) and only use Exception as a last resort with logging Confidence: 75% Rule: 🟡 MEDIUM - O(n*m) lookup in findLastNode with edges.some() on every call (4 occurrences)Category: performance 📍 View all locations
Rule: 🟡 MEDIUM - Unreachable isinstance check for list typeCategory: quality File: Description: Line 561 checks if a value is a list, but langflow_component_path is retrieved from os.getenv which always returns str or None, making the list check unreachable. Suggestion: Remove the isinstance(langflow_component_path, list) check since os.getenv always returns str or None Confidence: 90% Rule: 🟡 MEDIUM - Validator with environment side effectCategory: quality File: Description: The field validator set_user_agent modifies os.environ as a side effect. Field validators should be pure functions without side effects. Suggestion: Move the os.environ modification to model_post_init or a separate initialization method Confidence: 75% Rule: 🟡 MEDIUM - Function attribute used for cachingCategory: quality File: Description: The function get_cache_service uses a mutable function attribute _factory for caching. This is not a standard Python pattern and could lead to unexpected behavior in multi-threaded environments. Suggestion: Consider using @lru_cache decorator or a proper singleton pattern instead of function attributes for caching Confidence: 65% Rule: 🟡 MEDIUM - DELETE endpoint returns 204 but includes response bodyCategory: bug File: Description: The DELETE endpoint specifies status_code=204 (No Content), but returns a response body with {"message": "Messages deleted successfully"}. According to HTTP standards, a 204 response MUST NOT include a message body. Suggestion: Remove the return statement at line 190, or change the status code to 200 if you want to return a confirmation message. For 204 responses, simply return None. Confidence: 95% Rule: 🟡 MEDIUM - Variable shadowing - 'col' reusedCategory: quality File: Description: The variable 'col' is imported from sqlmodel at line 11 and then redefined at line 41, shadowing the import within the function scope. Suggestion: Use a different variable name like 'order_column' or 'sort_col' to avoid shadowing the imported 'col' function. Confidence: 85% Rule: 🟡 MEDIUM - setTimeout missing cleanup in component (2 occurrences)Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - JSON.parse without try-catch and validation (2 occurrences)Category: security 📍 View all locations
Rule: 🟡 MEDIUM - Using empty object type {} instead of proper typeCategory: quality File: Description: The 'tweaksBuildedObject' prop is typed as '{}' (empty object type), which provides minimal type safety. Suggestion: Define a proper interface for tweaksBuildedObject, or use 'Record<string, unknown>' for dynamic key-value objects. Confidence: 75% Rule: 🔵 LOW - Unused variable with underscore prefixCategory: quality File: Description: Variable '_fileContent' is declared but never used. The underscore prefix indicates intentional unused variable. Suggestion: Either use the variable in the test or remove the line entirely if not needed Confidence: 60% Rule: 🔵 LOW - External API call without retry logicCategory: security File: Description: The is_valid_ollama_url method makes an HTTP GET request without retry logic. Transient network failures will cause false negatives in URL validation. Suggestion: Consider adding retry logic with exponential backoff using tenacity for improved reliability, or document that transient failures may return false. Confidence: 65% Rule: Powered by diffray - Multi-Agent Code Review Agent |
Changes SummaryThis PR merges changes from the 1.7.1 release branch into main, featuring a performance optimization for get_cache_service() (10.9x speedup via factory caching), security improvements for message endpoint authorization (user flow filtering), bug fixes in API utils and memory handling, and various test improvements. It also includes version bumps to 1.7.1 across all packages. Type: mixed Components Affected: Backend cache service (deps.py), Backend monitor API (authorization), Backend memory handling, API utilities (remove_api_keys fix), LFX CLI and settings, Frontend embed modal, Frontend MCP server tab, GitHub workflow configurations, Starter project templates Architecture Impact
Risk Areas: Authorization change in monitor.py - messages and sessions now filtered by user's flows, could affect existing behavior, Factory caching in get_cache_service could cause issues if factory state needs to be reset between tests, Memory.py changes with asyncio.iscoroutine checks may have edge cases with sync/async compatibility, LFX settings change for database path resolution fallback logic Suggestions
Full review in progress... | Powered by diffray |
| </div> | ||
| </BaseModal.Header> | ||
| <BaseModal.Content className=""> | ||
| <div className="relative flex h-full w-full"> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={copyToClipboard} | ||
| data-testid="btn-copy-code" | ||
| className="!hover:bg-foreground group absolute right-2 top-2" | ||
| > | ||
| {isCopied ? ( | ||
| <IconComponent | ||
| name="Check" | ||
| className="h-5 w-5 text-muted-foreground" | ||
| /> | ||
| ) : ( | ||
| <IconComponent | ||
| name="Copy" | ||
| className="!h-5 !w-5 text-muted-foreground" | ||
| /> | ||
| )} | ||
| </Button> | ||
| <SyntaxHighlighter | ||
| showLineNumbers={true} | ||
| wrapLongLines={true} | ||
| language="html" | ||
| style={isDark ? oneDark : oneLight} | ||
| className="!mt-0 h-full w-full overflow-scroll !rounded-b-md border border-border text-left !custom-scroll" | ||
| > | ||
| {embedCode} | ||
| </SyntaxHighlighter> | ||
| </div> | ||
| <> | ||
| <CustomAPIGenerator isOpen={open} isEmbedded={true} /> | ||
| <div className="relative flex h-full w-full"> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={copyToClipboard} | ||
| data-testid="btn-copy-code" | ||
| className="!hover:bg-foreground group absolute right-2 top-2" | ||
| > |
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.
🟡 MEDIUM - Unhandled promise rejection in clipboard operation
Category: bug
Description:
The navigator.clipboard.writeText() operation can fail but the error is silently ignored. If the promise rejects, the user will see no feedback.
Suggestion:
Add error handling: .then(() => { /* success / }, (error) => { / show error message */ })
Confidence: 75%
Rule: bug_missing_try_catch
| stmt = stmt.order_by(order_col) | ||
| messages = await session.exec(stmt) | ||
| return [MessageResponse.model_validate(d, from_attributes=True) for d in messages] | ||
| except Exception as e: |
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.
🔴 CRITICAL - Missing resource-level authorization on message deletion
Category: security
Description:
The DELETE /messages endpoint only checks authentication but doesn't verify users own the messages being deleted. An attacker could delete other users' messages.
Suggestion:
Add authorization logic to verify the current user owns the messages before deletion by checking if message.flow_id belongs to the user's flows.
Confidence: 95%
Rule: soc2_rbac_least_privilege
| stmt = stmt.order_by(order_col) | ||
| messages = await session.exec(stmt) | ||
| return [MessageResponse.model_validate(d, from_attributes=True) for d in messages] | ||
| except Exception as e: |
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.
🔴 CRITICAL - Missing resource-level authorization on message update
Category: security
Description:
The PUT /messages/{message_id} endpoint only checks authentication but doesn't verify the user owns the message before updating it.
Suggestion:
Add authorization check to verify message ownership by checking if the message's flow_id belongs to a flow owned by current_user.
Confidence: 95%
Rule: soc2_rbac_least_privilege
| from langflow.services.database.models.user.model import User | ||
| from langflow.services.database.models.vertex_builds.crud import ( | ||
| delete_vertex_builds_by_flow_id, | ||
| get_vertex_builds_by_flow_id, |
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.
🟠 HIGH - Missing authorization on vertex builds endpoint
Category: security
Description:
The GET /builds endpoint accepts a flow_id parameter but only checks authentication. It doesn't verify the user owns the flow before returning vertex builds.
Suggestion:
Add authorization check to verify flow ownership: check if flow.user_id == current_user.id before returning data.
Confidence: 90%
Rule: soc2_rbac_least_privilege
| raise HTTPException(status_code=500, detail=str(e)) from e | ||
|
|
||
|
|
||
| @router.get("/messages/sessions", dependencies=[Depends(get_current_active_user)]) | ||
| @router.get("/messages/sessions") | ||
| async def get_message_sessions( | ||
| session: DbSession, |
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.
🟠 HIGH - Missing authorization on vertex builds deletion
Category: security
Description:
The DELETE /builds endpoint accepts a flow_id parameter but only checks authentication. It doesn't verify the user owns the flow before deleting its vertex builds.
Suggestion:
Add authorization check to verify flow ownership before deletion.
Confidence: 90%
Rule: soc2_rbac_least_privilege
| ).toBeTruthy(); | ||
| } | ||
|
|
||
| // Copy configuration |
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.
🟡 MEDIUM - Non-null assertion operator used without null check
Category: quality
Description:
Uses sseUrlMatch![1] assuming regex match succeeded. While preceded by expect(sseUrlMatch).not.toBeNull(), using ! is still fragile.
Suggestion:
Use safer pattern: expect(sseUrlMatch).not.toBeNull(); const sseUrl = sseUrlMatch?.[1] ?? '';
Confidence: 70%
Rule: quality_missing_project_pattern
| McpJsonContent: () => <div data-testid="json-content" />, | ||
| // Mock react-syntax-highlighter for McpJsonContent | ||
| jest.mock("react-syntax-highlighter", () => ({ | ||
| // biome-ignore lint/suspicious/noExplicitAny: TODO We need to fix this. added suppression for 1.7.1 merge |
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.
🔵 LOW - biome-ignore directive with TODO
Category: quality
Description:
Line 149 contains biome-ignore for 'suspicious/noExplicitAny' with TODO note, indicating known technical debt.
Suggestion:
Fix the type safety issue by providing proper types, or track as technical debt with issue reference.
Confidence: 70%
Rule: quality_missing_project_pattern
| if output_json.get("success"): | ||
| result_text = str(output_json.get("result", "")) | ||
| # The agent should compute 15 * 7 = 105 | ||
| assert "105" in result_text, f"Expected 105 in result: {result_text}" |
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.
🟡 MEDIUM - Conditional logic in test that could hide test failures
Category: quality
Description:
Test uses conditional logic 'if output_json.get("success")' to decide whether to check the result. This could cause the test to pass silently when the flow fails.
Suggestion:
Remove the conditional and always assert that 'success' is True. Then verify the result content:
assert output_json.get("success") is True, f"Execution failed: {output_json}"
result_text = str(output_json.get("result", ""))
assert "105" in result_text, f"Expected 105 in result: {result_text}"
Confidence: 85%
Rule: test_no_conditionals
| const isGenerateButtonVisible = await generateApiKeyButton | ||
| .isVisible() | ||
| .catch(() => false); | ||
|
|
||
| if (isGenerateButtonVisible) { | ||
| // Get the JSON configuration before generating | ||
| const preElement = page.locator("pre").first(); | ||
| const jsonBeforeGeneration = await preElement.textContent(); | ||
|
|
||
| // Verify "YOUR_API_KEY" is present in the JSON before generation | ||
| expect(jsonBeforeGeneration).toContain("YOUR_API_KEY"); | ||
|
|
||
| // Verify the button is visible and clickable | ||
| await expect(generateApiKeyButton).toBeVisible(); | ||
| await expect(generateApiKeyButton).toBeEnabled(); | ||
|
|
||
| // Click the Generate API key button | ||
| await generateApiKeyButton.click(); | ||
|
|
||
| // Wait for the API key to be generated and verify the state change | ||
| // The button text should change from "Generate API key" to "API key generated" | ||
| await expect(page.getByText("API key generated")).toBeVisible({ | ||
| timeout: 5000, | ||
| }); | ||
|
|
||
| // Wait for the JSON to update - it should no longer contain "YOUR_API_KEY" | ||
| // Retry until the JSON no longer contains "YOUR_API_KEY" | ||
| let jsonAfterGeneration = await preElement.textContent(); | ||
| let retries = 0; | ||
| while ( | ||
| jsonAfterGeneration?.includes("YOUR_API_KEY") && | ||
| retries < 10 | ||
| ) { | ||
| await page.waitForTimeout(500); | ||
| jsonAfterGeneration = await preElement.textContent(); | ||
| retries++; | ||
| } | ||
|
|
||
| // Verify "YOUR_API_KEY" is no longer present | ||
| expect(jsonAfterGeneration).not.toContain("YOUR_API_KEY"); | ||
|
|
||
| // Verify that an actual API key (not "YOUR_API_KEY") is present | ||
| // The JSON format has: "--headers", "x-api-key", "<api-key-value>" | ||
| // Match for "x-api-key" followed by comma and whitespace/newlines, then the API key | ||
| const apiKeyMatch = jsonAfterGeneration?.match( | ||
| /"x-api-key"[\s,]*"([^"]+)"/, | ||
| ); | ||
| expect(apiKeyMatch).not.toBeNull(); | ||
| if (apiKeyMatch) { | ||
| const generatedApiKey = apiKeyMatch[1]; | ||
| expect(generatedApiKey).not.toBe("YOUR_API_KEY"); | ||
| expect(generatedApiKey.length).toBeGreaterThan(0); | ||
| // API keys should be non-empty strings | ||
| expect(generatedApiKey.trim().length).toBeGreaterThan(0); | ||
| } | ||
|
|
||
| // Verify the Generate API key button text is no longer visible | ||
| // (it should be replaced by "API key generated") | ||
| await expect(generateApiKeyButton).not.toBeVisible(); | ||
| } else { | ||
| // If button is not visible, verify we're in a valid state: | ||
| // Either "API key generated" is shown (already generated) or | ||
| // we're in auto-login mode (no API key needed) | ||
| const apiKeyGeneratedText = page.getByText("API key generated"); | ||
| const hasApiKeyGenerated = await apiKeyGeneratedText | ||
| .isVisible() | ||
| .catch(() => false); | ||
|
|
||
| // In auto-login mode, neither button should be visible, which is expected | ||
| // In API key mode with key already generated, "API key generated" should be visible | ||
| expect( | ||
| hasApiKeyGenerated || | ||
| !(await page.getByText("Generate API key").isVisible()), | ||
| ).toBeTruthy(); | ||
| } |
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.
🟡 MEDIUM - Conditional test execution based on button visibility
Category: quality
Description:
Test uses if-else to conditionally execute different assertions based on whether a button is visible. This means the test could pass or skip important validations depending on runtime state.
Suggestion:
Split into two separate tests: one for when API key needs generation, one for when it's already generated. Use test fixtures to set up the required state.
Confidence: 80%
Rule: test_no_conditionals
| import { CustomAPIGenerator } from "@/customization/components/custom-api-generator"; | ||
| import { useDarkStore } from "@/stores/darkStore"; | ||
| import IconComponent from "../../components/common/genericIconComponent"; | ||
| import { Button } from "../../components/ui/button"; |
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.
🟡 MEDIUM - Using empty object type '{}' instead of proper type
Category: quality
Description:
The 'tweaksBuildedObject' parameter uses the empty object type '{}' which provides almost no type safety.
Suggestion:
Replace '{}' with a proper type definition like 'Record<string, unknown>' for better type safety, or define a specific interface.
Confidence: 85%
Rule: ts_object_type_vs_record
Review Summary
Validated 93 issues: 55 kept, 38 filtered Issues Found: 55See 20 individual line comment(s) for details. 📊 29 unique issue type(s) across 55 location(s) 📋 Full issue list (click to expand)🔴 CRITICAL - Missing resource-level authorization on message deletion (6 occurrences)Category: security 📍 View all locations
Rule: 🟠 HIGH - Infinite loop risk - counter never incrementsCategory: bug File: Description: Variable 'count' is declared as const and used in a while loop condition (count < 20), but it is never incremented inside the loop. This will cause an infinite loop. Suggestion: Change 'const count = 0' to 'let count = 0' and add 'count++' inside the while loop body. Confidence: 100% Rule: 🟠 HIGH - Retry logic wrapping entire test body hides root causesCategory: quality File: Description: The entire test is wrapped in a for-loop retry mechanism (maxRetries = 5), which attempts to run the entire 400-line test up to 5 times. This approach hides intermittent failures. Suggestion: Remove the retry wrapper around the entire test. If specific steps are flaky, add targeted retry logic only for those steps, or better yet, fix the underlying timing/synchronization issues. Confidence: 85% Rule: 🟠 HIGH - CancelledError caught and converted to ValueError with retry (2 occurrences)Category: bug 📍 View all locations
Rule: 🟠 HIGH - Timer not cleared on component unmount (2 occurrences)Category: bug 📍 View all locations
Rule: 🟠 HIGH - Async forEach without proper error handlingCategory: bug File: Description: processFlows uses forEach with async callback but function doesn't await. The await inside forEach doesn't actually wait for completion. Suggestion: Use Promise.all with map instead of forEach to properly handle async operations: const results = await Promise.all(DbData.map(async (flow) => { ... })); Confidence: 92% Rule: 🟡 MEDIUM - Unhandled promise rejection in clipboard operationCategory: bug File: Description: The navigator.clipboard.writeText() operation can fail but the error is silently ignored. If the promise rejects, the user will see no feedback. Suggestion: Add error handling: .then(() => { /* success / }, (error) => { / show error message */ }) Confidence: 75% Rule: 🟡 MEDIUM - Silent failure in clipboard copy operationCategory: bug File: Description: The copyToClipboard function silently ignores clipboard write failures with an empty arrow function () => {}. Users will not be notified if the copy operation fails. Suggestion: Add error handling: (error) => { setErrorData({ title: 'Failed to copy', list: [error.message] }); } Confidence: 85% Rule: 🟡 MEDIUM - Exception attribute access without proper type checkingCategory: bug File: Description: The code accesses e.response.get() assuming the exception has a 'response' attribute that is a dictionary. If e.response exists but is not a dict, this will raise an AttributeError. Suggestion: Add type checking: if hasattr(e, 'response') and isinstance(e.response, dict) and e.response.get('Error', {}).get('Code') == 'NoSuchKey' Confidence: 70% Rule: 🟡 MEDIUM - console.warn used in test code (5 occurrences)Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Test marked xfail indicating silent_errors bug (2 occurrences)Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Sequential API calls in loop for model capabilitiesCategory: performance File: Description: The get_models() method makes an API call for each model inside the for loop to fetch capabilities. With N models, this results in N sequential HTTP requests. Suggestion: Use asyncio.gather() to make concurrent API requests for all models instead of sequential awaits. This would reduce total time from O(N) sequential calls to O(1) concurrent calls. Confidence: 85% Rule: 🟡 MEDIUM - Bare except with noqa suppresses linting (4 occurrences)Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Conditional await using asyncio.iscoroutine() (2 occurrences)Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Playwright in production dependenciesCategory: quality File: Description: Playwright is a testing framework listed in 'dependencies' instead of 'devDependencies'. Increases production bundle size unnecessarily. Suggestion: Move 'playwright' from 'dependencies' to 'devDependencies' to reduce production bundle size. Confidence: 95% Rule: 🟡 MEDIUM - Using loose equality for boolean comparison in SQLAlchemyCategory: bug File: Description: Uses Suggestion: Change Confidence: 75% Rule: 🟡 MEDIUM - Console statements in production utility code (2 occurrences)Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Conditional logic in test that could hide test failures (5 occurrences)Category: quality 📍 View all locations
as... | 85% | Rule: 🟡 MEDIUM - Using empty object type '{}' instead of proper typeCategory: quality File: Description: The 'tweaksBuildedObject' parameter uses the empty object type '{}' which provides almost no type safety. Suggestion: Replace '{}' with a proper type definition like 'Record<string, unknown>' for better type safety, or define a specific interface. Confidence: 85% Rule: 🟡 MEDIUM - Empty collection edge case not handled - division by zero (2 occurrences)Category: bug 📍 View all locations
Rule: 🔵 LOW - Mismatch between documented and actual parameter typeCategory: docs File: Description: The docstring documents the 'default' parameter as 'ServiceFactory' type, but the actual function signature shows 'default=None' with no type hint. The parameter is used generically. Suggestion: Update docstring to reflect the actual parameter type: 'default (Any, optional): Default value to return if service is not found. Defaults to None.' or add type hint to the function signature. Confidence: 75% Rule: 🔵 LOW - Import statement inside validator function (2 occurrences)Category: quality 📍 View all locations
Rule: 🔵 LOW - Function attribute used for caching without documentationCategory: quality File: Description: Using function attributes (_factory) for caching is non-standard. While functional, it could cause confusion or thread-safety issues. Suggestion: Add a comment explaining the caching pattern, or use functools.lru_cache or a module-level variable for cleaner implementation. Confidence: 70% Rule: 🔵 LOW - Import statement inside try blockCategory: quality File: Description: The 'warnings' import is inside the try block. While functional, top-level imports are preferred for clarity and performance. Suggestion: Move 'import warnings' to the top of the file. This is a minor style issue. Confidence: 65% Rule: 🔵 LOW - Verbosity constants defined but potential non-useCategory: quality File: Description: VERBOSITY_DETAILED = 2 and VERBOSITY_FULL = 3 constants are defined. Need to verify they're used instead of magic numbers. Suggestion: Ensure these constants are used throughout the file instead of hardcoded values 2 and 3. Confidence: 60% Rule: 🔵 LOW - Unused variable _fileContent (2 occurrences)Category: quality 📍 View all locations
Rule: 🔵 LOW - Dynamic column ordering needs validationCategory: security File: Description: Uses getattr(MessageTable, order_by) to dynamically construct ORDER BY clause from user input. While exploitability is limited by SQLAlchemy ORM, explicit allowlist is better. Suggestion: Add explicit validation: ALLOWED_ORDER_COLUMNS = {'timestamp', 'sender', 'sender_name'} Confidence: 65% Rule: 🔵 LOW - Using 'any' type disables type checking (2 occurrences)Category: quality 📍 View all locations
Rule: 🔵 LOW - Using 'any' return type disables type checking (2 occurrences)Category: quality 📍 View all locations
Rule: Powered by diffray - Multi-Agent Code Review Agent |
⚡️ This pull request contains optimizations for PR #11129
If you approve this dependent PR, these changes will be merged into the original PR branch
aka-merge-1.7.1.📄 1,090% (10.90x) speedup for
get_cache_serviceinsrc/backend/base/langflow/services/deps.py⏱️ Runtime :
2.72 milliseconds→229 microseconds(best of198runs)📝 Explanation and details
The optimization caches the
CacheServiceFactoryinstance using function attributes, eliminating redundant object creation on every call toget_cache_service().Key changes:
CacheServiceFactory()instance on every call, the optimization useshasattr()to check if a cached factory exists on the function object itself (get_cache_service._factory). The factory is only created once and reused for subsequent calls.CacheServiceFactoryis moved inside the conditional block, so it only executes once when the factory is first created.Why this leads to speedup:
CacheServiceFactoryobject 34 times (per the profiler), while the optimized version creates it only once.CacheServiceFactory()constructor calls after the first execution.Performance impact:
The line profiler shows the optimized version reduces total time for
get_cache_service()from 78.7ms to 76.5ms, with most calls now hitting the cached path. The 1090% speedup suggests this function is called frequently in a hot path, making the per-call savings compound significantly.Test case effectiveness:
The optimization works particularly well for test cases that make repeated calls to
get_cache_service(), such as the singleton tests and large-scale tests that verify cache operations multiple times. The caching ensures consistent factory reuse across all these calls.✅ Correctness verification report:
🌀 Click to see Generated Regression Tests
To edit these changes
git checkout codeflash/optimize-pr11129-2025-12-22T21.28.23and push.