fix: add .ts and .mjs to EditorConfig indent rules#28396
Conversation
…eness
The previous implementation used only node_id as the Redis cache key,
which could cause conflicts when different apps have nodes with the
same node_id. This fix adds app_id to the cache key pattern to ensure
uniqueness across different applications.
Changes:
- trigger_service.py: Updated cache key from {key}:{node_id} to {key}:{app_id}:{node_id}
- webhook_service.py: Applied the same fix for webhook trigger nodes
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
The .editorconfig was missing indent configuration for .ts (TypeScript) and .mjs (ES modules) files. This could lead to inconsistent formatting across the codebase.
dosubot
Bot
added
the
size:S
Summary of ChangesHello @17hz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses two distinct areas: improving code formatting consistency and enhancing Redis cache management. It extends the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request combines two unrelated changes. The first is a configuration update to .editorconfig to include .ts and .mjs files for consistent formatting, which is a good improvement.
The second, more significant change, is a critical bug fix in trigger_service.py and webhook_service.py. It correctly scopes Redis cache keys with the application ID to prevent cache collisions between different apps. This is an important fix.
However, I have two main concerns:
- Missing Tests: The PR description claims tests have been added, but I don't see any for the cache key changes. Given the critical nature of this fix, it's essential to add integration tests to verify that caches for different apps are now properly isolated.
- Misleading PR Description: The PR title and description only mention the
.editorconfigchange, completely omitting the bug fix. This makes it hard to understand the full scope of the changes.
I've left detailed comments on the need for tests. Ideally, these two changes should be in separate pull requests to follow the principle of atomic changes. Please consider updating the PR description to accurately reflect all changes.
| node_id = node_info["node_id"] | ||
| # firstly check if the node exists in cache | ||
| if not redis_client.get(f"{cls.__PLUGIN_TRIGGER_NODE_CACHE_KEY__}:{node_id}"): | ||
| if not redis_client.get(f"{cls.__PLUGIN_TRIGGER_NODE_CACHE_KEY__}:{app.id}:{node_id}"): |
There was a problem hiding this comment.
This change correctly scopes the cache key to the application, fixing a potential cache collision bug. However, the PR does not include tests to verify this fix. Please add an integration test that creates nodes with the same ID in two different apps and confirms that their caches are handled independently without collisions.
On a related note for maintainability, the Redis cache key format is now constructed in multiple places within this method (lines 213, 258, 288, 298). Consider centralizing this logic to prevent future inconsistencies.
| for node_id in nodes_id_in_graph: | ||
| # firstly check if the node exists in cache | ||
| if not redis_client.get(f"{cls.__WEBHOOK_NODE_CACHE_KEY__}:{node_id}"): | ||
| if not redis_client.get(f"{cls.__WEBHOOK_NODE_CACHE_KEY__}:{app.id}:{node_id}"): |
There was a problem hiding this comment.
This is a crucial fix to prevent cache collisions. However, similar to trigger_service.py, this change is not covered by tests. Please add an integration test to ensure that webhook node caches for different apps are properly isolated.
For maintainability, consider centralizing the cache key generation logic, as it's repeated on lines 815, 848, and 855.
|
Closing - branch was created from wrong base. Will recreate from main. |
1 participant
Summary
The
.editorconfigfile was missing indent configuration for.ts(TypeScript) and.mjs(ES modules) files. Previously, only.jsand.tsxfiles were covered, which could lead to inconsistent formatting across the codebase.This PR updates the configuration from
[*.{js,tsx}]to[*.{js,ts,tsx,mjs}]to ensure all JavaScript/TypeScript file variants follow the same 2-space indentation rules.Screenshots
N/A - Configuration file change only
Checklist
dev/reformat(backend) andcd web && npx lint-staged(frontend) to appease the lint gods