fix: remove lodash dependency from frontend to reduce bundle size#258
fix: remove lodash dependency from frontend to reduce bundle size#258jmanhype wants to merge 20 commits intoautomagik-dev:devfrom
Conversation
Summary of ChangesHello @jmanhype, 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 focuses on optimizing the frontend application's performance by eliminating 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 successfully removes the lodash dependency to reduce bundle size, which is a great improvement. The introduction of native JavaScript alternatives in frontend/src/utils/object.ts is a good approach. However, I've found some critical issues in the new implementations. Both isEqual and deepMerge do not handle circular references, which can lead to stack overflows and crash the application. Additionally, the isEqual function incorrectly compares Set objects, failing to perform a deep equality check on their contents. These issues should be addressed to ensure the stability and correctness of these utility functions.
| export function isEqual(a: unknown, b: unknown): boolean { | ||
| if (a === b) return true; | ||
|
|
||
| if (a === null || b === null) return a === b; | ||
| if (typeof a !== typeof b) return false; | ||
|
|
||
| if (typeof a !== 'object') return a === b; | ||
|
|
||
| // Handle Date | ||
| if (a instanceof Date && b instanceof Date) { | ||
| return a.getTime() === b.getTime(); | ||
| } | ||
|
|
||
| // Handle RegExp | ||
| if (a instanceof RegExp && b instanceof RegExp) { | ||
| return a.toString() === b.toString(); | ||
| } | ||
|
|
||
| // Handle Map | ||
| if (a instanceof Map && b instanceof Map) { | ||
| if (a.size !== b.size) return false; | ||
| for (const [key, val] of a) { | ||
| if (!b.has(key) || !isEqual(val, b.get(key))) return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| // Handle Set | ||
| if (a instanceof Set && b instanceof Set) { | ||
| if (a.size !== b.size) return false; | ||
| for (const val of a) { | ||
| if (!b.has(val)) return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| // Handle Arrays | ||
| if (Array.isArray(a) && Array.isArray(b)) { | ||
| if (a.length !== b.length) return false; | ||
| for (let i = 0; i < a.length; i++) { | ||
| if (!isEqual(a[i], b[i])) return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| // Handle plain objects | ||
| if (Array.isArray(a) !== Array.isArray(b)) return false; | ||
|
|
||
| const aObj = a as Record<string, unknown>; | ||
| const bObj = b as Record<string, unknown>; | ||
|
|
||
| const keysA = Object.keys(aObj); | ||
| const keysB = Object.keys(bObj); | ||
|
|
||
| if (keysA.length !== keysB.length) return false; | ||
|
|
||
| for (const key of keysA) { | ||
| if (!Object.prototype.hasOwnProperty.call(bObj, key)) return false; | ||
| if (!isEqual(aObj[key], bObj[key])) return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
The isEqual function does not handle circular references in objects or arrays. This can lead to an infinite loop and a stack overflow error if it's used to compare objects with cycles. For example, const a = {}; a.a = a; isEqual(a, a); would crash.
Lodash's isEqual handles this by keeping track of visited objects. A similar mechanism (e.g., using a Map or Set to store visited object pairs) should be implemented to prevent infinite recursion.
| export function deepMerge<T extends Record<string, unknown>>( | ||
| ...objects: (Partial<T> | undefined | null)[] | ||
| ): T { | ||
| const result: Record<string, unknown> = {}; | ||
|
|
||
| for (const obj of objects) { | ||
| if (!obj) continue; | ||
|
|
||
| for (const key of Object.keys(obj)) { | ||
| const targetVal = result[key]; | ||
| const sourceVal = obj[key]; | ||
|
|
||
| if (isPlainObject(targetVal) && isPlainObject(sourceVal)) { | ||
| result[key] = deepMerge( | ||
| targetVal as Record<string, unknown>, | ||
| sourceVal as Record<string, unknown> | ||
| ); | ||
| } else { | ||
| result[key] = sourceVal; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return result as T; | ||
| } |
There was a problem hiding this comment.
Similar to isEqual, the deepMerge function does not handle circular references. This will cause an infinite loop and a stack overflow if an object with a cycle is passed to it. For example: const a = {}; a.a = a; deepMerge({}, a); will crash.
To prevent this, you should track visited objects during the merge process, for example by using a WeakMap. When a visited object is encountered again, you can use its already merged counterpart from the cache instead of recursing.
frontend/src/utils/object.ts
Outdated
| // Handle Set | ||
| if (a instanceof Set && b instanceof Set) { | ||
| if (a.size !== b.size) return false; | ||
| for (const val of a) { | ||
| if (!b.has(val)) return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The implementation for comparing Set objects is incorrect for sets containing objects. The b.has(val) check uses Same-Value-Zero equality, which is similar to ===, and will not perform a deep equality check for objects within the set.
For example, isEqual(new Set([{a: 1}]), new Set([{a: 1}])) will incorrectly return false.
To fix this, you need to perform a deep comparison of elements. One way to do this is to convert one set to an array and, for each element in the first set, find and remove a deep-equal element from that array. If any element from the first set cannot be found, the sets are not equal.
| export function deepMerge<T extends Record<string, unknown>>( | ||
| ...objects: (Partial<T> | undefined | null)[] | ||
| ): T { | ||
| const result: Record<string, unknown> = {}; | ||
|
|
||
| for (const obj of objects) { | ||
| if (!obj) continue; | ||
|
|
||
| for (const key of Object.keys(obj)) { | ||
| const targetVal = result[key]; | ||
| const sourceVal = obj[key]; | ||
|
|
||
| if (isPlainObject(targetVal) && isPlainObject(sourceVal)) { | ||
| result[key] = deepMerge( | ||
| targetVal as Record<string, unknown>, | ||
| sourceVal as Record<string, unknown> | ||
| ); | ||
| } else { | ||
| result[key] = sourceVal; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return result as T; | ||
| } |
There was a problem hiding this comment.
The comment correctly states that "Arrays are replaced, not merged." This is a different behavior from lodash.merge, which can merge array elements if they are objects. For example, _.merge([{ 'a': 1 }], [{ 'b': 2 }]) results in [{ 'a': 1, 'b': 2 }], while this implementation would result in [{ 'b': 2 }].
While this might be acceptable for the current use cases in this repository, it's a significant difference for a general-purpose utility function. This could lead to unexpected behavior if other developers use this function expecting lodash-like behavior.
Consider either renaming the function to something like deepMergeAndReplaceArrays to make the behavior explicit, or updating the implementation to more closely match lodash.merge's array handling if that is desired.
Split the monolithic 1680-line router.rs into modular route files: - routes/state.rs: ForgeAppState and related implementations - routes/tasks.rs: Task creation, listing, and WebSocket streaming - routes/attempts.rs: Task attempt handlers with forge overrides - routes/config.rs: Global forge configuration endpoints - routes/projects.rs: Project-specific settings and profiles - routes/omni.rs: Omni service status and notifications - routes/agents.rs: Forge agents and neuron management - routes/frontend.rs: Static file serving and documentation The main router.rs is now a thin ~145-line composition layer that imports and composes these modules. Also added build.rs to create dummy frontend/dist directory for compilation. Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
- Enhanced ThemeProvider with localStorage persistence for immediate theme application on page load - Added resolvedTheme property to handle system preference resolution - Created ThemeToggle dropdown component with Light/Dark/System options - Integrated ThemeToggle into navbar for easy access - Added listener for system theme changes when in SYSTEM mode Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Implements a CLI command to clean up orphaned worktrees that are no longer referenced by active task attempts in the database. Features: - Scans temp directory for worktree directories - Queries database for active task_attempts with worktree paths - Identifies orphans (on disk but not in DB) - Shows orphan list with directory sizes - --force flag to delete orphans and show space recovered Usage: forge cleanup # Dry run - shows what would be deleted forge cleanup --force # Actually delete orphan worktrees Changes: - forge-app/src/bin/cleanup.rs: New cleanup binary - forge-app/Cargo.toml: Register new binary - npx-cli/bin/cli.js: Route cleanup subcommand - scripts/build/build.sh: Build and package cleanup binary Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
- Replace flatten() with explicit error handling for directory reads
- Refactor size_human() to static format_size() method
- Use to_string_lossy() instead of unwrap_or("unknown") for names
- Add TODO comment about duplicated database URL logic
- Make CLI argument parsing more robust by searching args array
Addresses high and medium severity feedback from code review.
Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Adds github_issue_id field to tasks to enforce the "No Wish Without Issue" rule from AGENTS.md. This links tasks to their originating GitHub issues for traceability. Changes: - Add runtime migration to add github_issue_id column to tasks table - Update ForgeCreateTask struct with github_issue_id field - Update forge_create_task to persist github_issue_id - Update forge_create_task_and_start with ForgeCreateAndStartTaskRequest - Update OpenAPI spec with github_issue_id documentation Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Wrap frequently re-rendered list item components with React.memo: - TaskCard (renders 10-50+ times in Kanban board) - DisplayConversationEntry (renders for every message in conversation) - DiffCard (renders once per changed file) - ProjectCard (renders in project grid) - TaskRelationshipCard (renders in relationship lists) Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Replace lodash imports (isEqual, cloneDeep, merge) with native JavaScript alternatives in ProjectSettings.tsx and GeneralSettings.tsx: - Add frontend/src/utils/object.ts with native implementations: - isEqual: deep equality comparison - deepClone: uses structuredClone API - deepMerge: custom deep merge implementation - Remove lodash from root package.json - Remove @types/lodash from frontend/package.json This reduces the bundle size by ~40KB. Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
jmanhype
force-pushed
the
claude/remove-lodash-dependency-01SgWKcZgCzV9w9bVB8v1SbL
branch
from
f7b448d to
a359a55
Compare
- Add WeakMap tracking to prevent stack overflow on circular objects - Fix Set deep equality to use deep comparison instead of shallow - Addresses gemini-code-assist critical safety issues gemini-code-assist review identified these critical bugs: 1. Missing circular reference handling could crash app 2. Set comparison used shallow equality instead of deep Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Fixes gemini-code-assist correctness issue where forge_create_task_and_start returned stale task data after updating github_issue_id field. - Change task to mut and re-fetch after UPDATE - Ensures returned task object has the updated github_issue_id gemini-code-assist review identified: API endpoint returns stale data after update Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Adds typeof window check before accessing window.matchMedia to prevent crashes during server-side rendering. Fixes gemini-code-assist critical SSR safety issue that could crash the application during SSR when theme is SYSTEM mode. - Check if window exists before matchMedia call - Default to 'light' theme during SSR Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Fixed two compilation issues discovered during dogfood testing: 1. Wrong API method (lines 111, 387): - Changed Task::get_one() to Task::find_by_id() - Added proper Option handling with ok_or() 2. Missing struct field (lines 210, 526): - Added attempt_count field to TaskWithAttemptStatus initializers - Line 210: attempt_count = 0 (list tasks) - Line 526: attempt_count = 1 (just created first attempt) These fixes ensure the code compiles and runs correctly with the merged PR automagik-dev#280 (GitHub Issue Tracking) changes. Related: Issue automagik-dev#289 (Task serialization follow-up) Tested: Dogfood testing on testing/all-gemini-fixes-merged branch 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
|
@codex review this pr |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let dep_clone = deployment.clone(); | ||
|
|
||
| // Forge override: config router with increased body limit for /profiles | ||
| router = router.merge(forge_config_router().with_state::<ForgeAppState>(dep_clone.clone())); | ||
| router = router.merge(config::forge_config_router().with_state::<ForgeAppState>(dep_clone.clone())); | ||
| router = |
There was a problem hiding this comment.
The new upstream_api_router builds a Router<ForgeAppState> but each upstream merge calls .with_state::<ForgeAppState>(dep_clone.clone()) where dep_clone is a DeploymentImpl. Router::with_state expects the actual state value of the declared type, so passing a DeploymentImpl here does not satisfy the Clone + Send + Sync bound for ForgeAppState and the code will not compile (expected struct ForgeAppState, found struct DeploymentImpl). With only DeploymentImpl available, none of the upstream routers can be constructed, so a cargo build of the backend fails immediately.
Useful? React with 👍 / 👎.
|
@codex review this pr |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let dep_clone = deployment.clone(); | ||
|
|
||
| // Forge override: config router with increased body limit for /profiles | ||
| router = router.merge(forge_config_router().with_state::<ForgeAppState>(dep_clone.clone())); | ||
| router = router.merge(config::forge_config_router().with_state::<ForgeAppState>(dep_clone.clone())); | ||
| router = |
There was a problem hiding this comment.
The upstream API router now calls .with_state::<ForgeAppState>(dep_clone.clone()) even though dep_clone is a DeploymentImpl. with_state stores the concrete state value you pass for the router’s state type, so this line will not type-check (a ForgeAppState is expected but a DeploymentImpl is provided), causing cargo build to fail before any routes can run. To compose the upstream routers under the Forge state, you need to pass the actual ForgeAppState (or keep these routers on DeploymentImpl and set the parent state later) rather than cloning DeploymentImpl here.
Useful? React with 👍 / 👎.
3 participants
Replace lodash imports (isEqual, cloneDeep, merge) with native JavaScript alternatives in ProjectSettings.tsx and GeneralSettings.tsx:
This reduces the bundle size by ~40KB.