feat: improved git graph data loading#110
Open
ruru-m07 wants to merge 10 commits into
Open
Conversation
- Added Recharts as a dependency in package.json and ui/package.json. - Introduced new CSS variables for chart colors in base.css. - Created a ChartContainer component to encapsulate chart logic and styling. - Implemented ChartTooltip and ChartLegend components for enhanced data representation. - Added utility functions to extract configuration from chart payloads.
📝 WalkthroughWalkthroughThis PR adds end-to-end commit activity tracking: Rust git log parsing gains shortstat support and ChangesCommit Activity + History Graph UI
Sequence DiagramsequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over HistoryGraph,GraphService: Commit Activity Flow
end
participant HistoryGraph
participant useCommitActivity
participant RepositoryState
participant TauriIPC as Tauri IPC
participant HistoryService
participant GraphService as graph_service
HistoryGraph->>useCommitActivity: mount with activityQuery
useCommitActivity->>RepositoryState: commit.commitActivity(query)
RepositoryState->>TauriIPC: invoke("commit_activity", {contextId, query})
TauriIPC->>HistoryService: commit_activity(query)
HistoryService->>GraphService: commit_activity(repo_path, query)
GraphService->>GraphService: pass1 — git log (hash+timestamp)
GraphService->>GraphService: pass2 — git log --shortstat
GraphService-->>HistoryService: CommitActivityResponse
HistoryService-->>TauriIPC: Ok(CommitActivityResponse)
TauriIPC-->>RepositoryState: CommitActivityResponse
RepositoryState-->>useCommitActivity: {items, headIndex, total}
useCommitActivity-->>HistoryGraph: data
HistoryGraph->>PanoramaChart: items, headIndex, totalCommits, scrollRef
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying gitru with
|
| Latest commit: |
adc89c7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dfa42349.gitru.pages.dev |
| Branch Preview URL: | https://ruru-git-graph-brush.gitru.pages.dev |
Owner
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (15)
crates/git/models/graph.rs (1)
79-91: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding Clone derive for convenience.
The
CommitActivityQuerystruct could benefit from derivingCloneto allow easy reuse of query objects without requiring ownership transfer.♻️ Optional enhancement
-#[derive(Debug, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize)] pub struct CommitActivityQuery {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git/models/graph.rs` around lines 79 - 91, Add the Clone trait to the derive macro for the CommitActivityQuery struct. Include Clone in the derive attribute alongside Debug, Serialize, and Deserialize to allow for easy cloning of query objects without requiring ownership transfer. Since the struct contains only primitive types (usize and bool), Clone can be derived automatically.crates/git/service/graph.rs (1)
801-820: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMutation inside
.map()closure is unconventional but functional.The
head_indexassignment inside the.map()closure works correctly but could be clearer. Consider using.enumerate().fold()or a separate loop for the HEAD matching to avoid side effects in a mapping operation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git/service/graph.rs` around lines 801 - 820, The head_index assignment inside the .map() closure is a side effect that should be avoided in a mapping operation. Refactor this code to separate the concerns: either use .enumerate().fold() to accumulate both the CommitActivityItem collection and the head_index value in a single pass, or extract the HEAD matching logic into a separate operation outside the map operation (such as finding the index where the oid matches head_oid.as_deref() after collecting the items). This will make the intent clearer and avoid mutating external state within the transformation closure.crates/git/service/history.rs (1)
57-63: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueBlocking call in async context.
graph_service::commit_activityis a synchronous function that performs multiple blockinggitsubprocess calls. Calling it directly from an async method could block the Tokio runtime thread. However, sincehistory_graphuses the same pattern and appears to work in production, this is likely acceptable for the current use case. Consider usingtokio::task::spawn_blockingif you observe async task starvation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git/service/history.rs` around lines 57 - 63, The synchronous `graph_service::commit_activity` function performs blocking git subprocess calls within the async `commit_activity` method, which could block the Tokio runtime thread. If async task starvation becomes an issue in production, wrap the `graph_service::commit_activity` call using `tokio::task::spawn_blocking` to offload the blocking work to a separate thread pool and prevent runtime starvation. This ensures the async context is not blocked by long-running synchronous operations.apps/desktop/src/components/historyGraph/columns/stats/stats-cell.tsx (1)
10-38: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd display name to memoized component.
React DevTools will show
Memo(Component)instead ofStatsCell, making debugging harder.♻️ Proposed fix
const StatsCell = React.memo(({ row }: StatsCellProps) => { // ... component body }); +StatsCell.displayName = "StatsCell";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/historyGraph/columns/stats/stats-cell.tsx` around lines 10 - 38, The StatsCell component is wrapped with React.memo() but does not have a display name assigned, which causes React DevTools to show it as Memo(Component) instead of StatsCell. Add a displayName property to the StatsCell constant after the React.memo() declaration by assigning StatsCell.displayName equal to the string 'StatsCell' to improve debugging visibility in React DevTools.apps/desktop/src/components/historyGraph/columns/branch/index.tsx (1)
6-27: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd display name to memoized component.
React DevTools will show
Memo(Component)instead ofBranch, making debugging harder.♻️ Proposed fix
const Branch = React.memo(({ rows, ...restProps }: GraphColumnsType) => { // ... component body }); +Branch.displayName = "Branch";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/historyGraph/columns/branch/index.tsx` around lines 6 - 27, The Branch component created with React.memo lacks a displayName property, causing React DevTools to display it as "Memo(Component)" instead of the actual component name. After the React.memo assignment for the Branch component, add a displayName property assignment to the Branch constant to explicitly set its display name to 'Branch'.apps/desktop/src/components/historyGraph/columns/branch/branch-cell.tsx (6)
17-65: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd display name to memoized component.
React DevTools will show
Memo(Component)instead ofBranchCell, making debugging harder.♻️ Proposed fix
const BranchCell = React.memo( ({ row, currentBranch, enablePushButton }: BranchCellProps) => { // ... component body }, ); +BranchCell.displayName = "BranchCell";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/historyGraph/columns/branch/branch-cell.tsx` around lines 17 - 65, The BranchCell component wrapped with React.memo does not have a display name assigned, causing React DevTools to display it as Memo(Component) instead of the actual component name. After the BranchCell assignment is complete with React.memo, add a displayName property assignment to the BranchCell variable with the value "BranchCell" to make the component identifiable in React DevTools for easier debugging.
114-126: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove unnecessary fragment wrapper.
The fragment around the local branch refs map serves no purpose.
♻️ Proposed fix
{row.localBranchRefs.length > 0 && ( - <> - {row.localBranchRefs.map((ref) => ( - <BranchBadge - key={ref.name} - ref={ref} - row={row} - currentBranch={currentBranch} - type="local" - /> - ))} - </> + row.localBranchRefs.map((ref) => ( + <BranchBadge + key={ref.name} + ref={ref} + row={row} + currentBranch={currentBranch} + type="local" + /> + )) )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/historyGraph/columns/branch/branch-cell.tsx` around lines 114 - 126, The fragment wrapper around the map function in the local branch refs rendering is unnecessary. Remove the empty fragment tags (the <> </>) that wrap the map call, and move the map function directly into the conditional expression where it renders the BranchBadge components for each ref in row.localBranchRefs.
158-159: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove commented-out code.
Commented backgroundColor properties should be removed if not needed.
♻️ Proposed fix
<div className="h-full w-0.5" style={{ - // backgroundColor: `color-mix(in oklab, ${row.color} 70%, var(--color-background))`, background: isCurrentBranch ? `repeating-linear-gradient(And:
style={{ height: 2, top: ROW_H / 2 - 2, width: "100%", - // backgroundColor: `color-mix(in oklab, ${row.color} 30%, var(--color-background))`, background: `repeating-linear-gradient(Also applies to: 179-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/historyGraph/columns/branch/branch-cell.tsx` around lines 158 - 159, Remove the commented-out backgroundColor property that uses color-mix in the branch-cell styling section. The line containing the commented code with backgroundColor and color-mix should be deleted entirely. Additionally, remove any other commented-out code at the other location mentioned in the review (line 179) that has similar styling comments. Clean up all unused commented code to keep the codebase maintainable.
127-139: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove unnecessary fragment wrapper.
The fragment around the remote refs map serves no purpose.
♻️ Proposed fix
{row.remoteRefs.length > 0 && ( - <> - {row.remoteRefs.map((ref) => ( - <BranchBadge - key={ref.name} - ref={ref} - row={row} - currentBranch={currentBranch} - type="remote" - /> - ))} - </> + row.remoteRefs.map((ref) => ( + <BranchBadge + key={ref.name} + ref={ref} + row={row} + currentBranch={currentBranch} + type="remote" + /> + )) )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/historyGraph/columns/branch/branch-cell.tsx` around lines 127 - 139, The fragment wrapper around the row.remoteRefs.map call is unnecessary since it only contains a single mapped element producing BranchBadge components. Remove the outer fragment delimiters (the empty angle brackets) while keeping the conditional check and the map function intact. This simplifies the JSX structure without changing the functionality or how the BranchBadge components are rendered with their key, ref, row, currentBranch, and type props.
43-59: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove unnecessary fragment wrapper.
The outer fragment
<>...</>serves no purpose here.♻️ Proposed fix
<div data-hidden-branch-refs className="hidden min-w-0 gap-1 items-center opacity-70" > {row.branchRefs.map((ref) => ( - <> - <BranchBadge - key={ref.name} - ref={ref} - row={row} - currentBranch={currentBranch} - type={ - ref.kind === "Local" - ? "local" - : ref.kind === "Remote" - ? "remote" - : "tag" - } - /> - </> + <BranchBadge + key={ref.name} + ref={ref} + row={row} + currentBranch={currentBranch} + type={ + ref.kind === "Local" + ? "local" + : ref.kind === "Remote" + ? "remote" + : "tag" + } + /> ))} </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/historyGraph/columns/branch/branch-cell.tsx` around lines 43 - 59, The fragment wrapper around the BranchBadge component in the branchRefs.map callback is unnecessary since there is only a single child element being rendered. Remove the outer empty fragment operators `<>` and `</>` that wrap the BranchBadge component, returning the BranchBadge component directly from the map function instead.
140-152: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove unnecessary fragment wrapper.
The fragment around the tags map serves no purpose.
♻️ Proposed fix
{row.tags.length > 0 && ( - <> - {row.tags.map((tag) => ( - <BranchBadge - key={tag.name} - ref={tag} - row={row} - currentBranch={currentBranch} - type="tag" - /> - ))} - </> + row.tags.map((tag) => ( + <BranchBadge + key={tag.name} + ref={tag} + row={row} + currentBranch={currentBranch} + type="tag" + /> + )) )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/historyGraph/columns/branch/branch-cell.tsx` around lines 140 - 152, The unnecessary fragment wrapper around the row.tags.map() call should be removed. Since the conditional check {row.tags.length > 0 && ...} already ensures the block only renders when tags exist, and the fragment is only wrapping a single map operation that generates BranchBadge components, you can safely remove the <> and </> fragment delimiters and render the map call directly within the conditional expression.apps/desktop/src/components/historyGraph/columns/commiters/index.tsx (1)
5-13: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd display name to memoized component.
React DevTools will show
Memo(Component)instead ofCommiters, making debugging harder.♻️ Proposed fix
const Commiters = React.memo(({ rows, ...restProps }: GraphColumnsType) => { // ... component body }); +Commiters.displayName = "Commiters";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/historyGraph/columns/commiters/index.tsx` around lines 5 - 13, The Commiters component is wrapped with React.memo but lacks a displayName property, causing React DevTools to display it as "Memo(Commiters)" instead of just "Commiters", which hampers debugging. Add a displayName property to the Commiters component after it is defined by assigning Commiters.displayName equal to the string "Commiters" to explicitly set how it appears in React DevTools.apps/desktop/src/components/historyGraph/columns/commiters/commiters-cell.tsx (2)
18-117: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd display name to memoized component.
React DevTools will show
Memo(Component)instead ofCommitersCell, making debugging harder.♻️ Proposed fix
const CommitersCell = React.memo(({ row }: CommitersCellProps) => { // ... component body }); +CommitersCell.displayName = "CommitersCell";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/historyGraph/columns/commiters/commiters-cell.tsx` around lines 18 - 117, The CommitersCell component is wrapped in React.memo but lacks a displayName property, causing React DevTools to display it as Memo(Component) instead of the actual component name. Add a displayName property assignment after the component export to set it to "CommitersCell", which will help with debugging and make the component easier to identify in React DevTools.
40-44: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExtract duplicate initials logic into helper function.
The initials fallback logic is duplicated three times. Extract it to reduce repetition.
♻️ Proposed refactor
At the top of the file or in a helper module:
const getInitials = (name: string) => name .split(" ") .map((n) => n[0]) .join("") .toUpperCase();Then replace all three occurrences:
<AvatarFallback> - {commitRow.commit?.authors.author.name - .split(" ") - .map((n) => n[0]) - .join("") - .toUpperCase()} + {getInitials(commitRow.commit?.authors.author.name)} </AvatarFallback>Also applies to: 68-72, 100-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/historyGraph/columns/commiters/commiters-cell.tsx` around lines 40 - 44, The initials extraction logic (splitting a name by spaces, taking the first character of each word, joining them, and converting to uppercase) is duplicated three times in the commiters-cell.tsx file at lines 40-44, 68-72, and 100-104. Create a reusable helper function called getInitials that takes a name string parameter and returns the formatted initials, then replace all three duplicate code blocks with calls to this new helper function to eliminate the repetition.apps/desktop/src/components/historyGraph/columns/stats/index.tsx (1)
5-13: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd display name to memoized component.
React DevTools will show
Memo(Component)instead ofStats, making debugging harder.♻️ Proposed fix
const Stats = React.memo(({ rows, ...restProps }: GraphColumnsType) => { // ... component body }); +Stats.displayName = "Stats";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/historyGraph/columns/stats/index.tsx` around lines 5 - 13, The Stats component is wrapped in React.memo but lacks a displayName property, which causes React DevTools to display it as Memo(Component) instead of Stats, making debugging difficult. After the Stats component definition (following the closing `});`), add a displayName property to the Stats component by assigning it the string value "Stats" to ensure proper identification in React DevTools.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/components/historyGraph/body.tsx`:
- Around line 142-150: The columns.map() function is using the array index idx
as the React key, which can cause component reuse issues if the columns array
order changes. Replace the key={idx} with a stable identifier from the column
object itself, such as a unique id property or column name that remains constant
regardless of array position. If the column objects don't have a unique
identifier, consider adding one or using a property that uniquely identifies
each column component.
- Around line 65-120: The useEffect hook is missing hoverClasses from its
dependency array, causing the event handlers in onMove to use stale references
if hoverClasses changes. Since hoverClasses is redeclared on every render (lines
43-46), the best fix is to move the hoverClasses constant outside the component
body so it's not recreated on each render, then reference it in the apply
function within the useEffect. Alternatively, if hoverClasses must remain in the
component, add it to the useEffect dependency array at the end, but this will
cause the effect to recreate on every render due to the constant being
redeclared.
- Around line 71-82: The `map` variable caches DOM query results indefinitely
without clearing stale entries when `rows` change, causing unbounded memory
growth. Add a useEffect hook that depends on `rows` and clears the `map` cache
by calling `map.clear()` whenever `rows` changes, ensuring the cache doesn't
accumulate stale row IDs and old DOM references.
In `@apps/desktop/src/components/historyGraph/columns/branch/branch-badge.tsx`:
- Around line 28-29: The conditional check at the ternary operator in
branch-badge.tsx is comparing currentBranch?.name with the result of finding a
Local ref's display_name. When both currentBranch is null and no Local ref
exists, both sides evaluate to undefined, causing undefined === undefined to be
true and incorrectly rendering the CircleDotDashed icon. Fix this by explicitly
ensuring that currentBranch is not null and a Local ref is found before
considering the branch as active, rather than relying on the loose comparison
that treats undefined === undefined as true. You should guard both conditions:
check that currentBranch is defined and check that the find() actually returns a
valid Local ref before doing the display_name comparison.
In
`@apps/desktop/src/components/historyGraph/columns/commiters/commiters-cell.tsx`:
- Line 37: The avatar URL in the commiters-cell component is exposing email
addresses as query parameters, which creates a PII leak risk that can be logged
by proxies, CDNs, or browser history. Replace the email query parameter in the
src attribute with either a hashed version of the email or a GitHub user ID if
available from the commitRow data. Apply this same change to all occurrences
mentioned (lines 37, 65, and 97) where the avatar URL is constructed with the
email parameter in the query string.
- Around line 93-108: The mapped `<div>` elements rendering coAuthor information
are missing required key props, which will cause React rendering warnings. Add a
unique key prop to the outer `<div>` element that wraps the Avatar and name
span. Use a unique identifier from the coAuthor object (such as coAuthor.email)
as the key value to ensure React can properly track and reconcile list items
during re-renders.
In `@apps/desktop/src/components/historyGraph/columns/lanes/lane-graph.tsx`:
- Around line 430-441: The AvatarImage component in the lane-graph.tsx file is
directly transmitting the commit author's email address as a query parameter to
the GitHub avatars endpoint, which exposes PII. Instead of sending the raw email
in the src URL of AvatarImage, hash the email address using MD5 (which is the
expected format for GitHub's avatar API) before passing it to the endpoint. This
way the email remains private while still allowing the avatar endpoint to
generate the appropriate avatar image.
In `@apps/desktop/src/components/historyGraph/columns/stats/stats-cell.tsx`:
- Around line 22-34: The code in the stats-cell component assumes
innerRow.commit.stats always exists and will crash if stats is null or undefined
when accessing properties like files_changed, insertions, and deletions. Add
null guard checks before rendering the statistics section and the DiffStat
component to safely handle cases where commit or stats are missing. Return early
or render a fallback UI (such as a placeholder or empty state) when
innerRow.commit.stats is not available.
In `@apps/desktop/src/components/historyGraph/columns/summary/index.tsx`:
- Around line 8-10: The `rows.map()` function rendering `SummaryCell` is missing
a `key` prop, which can cause incorrect reconciliation when the row set changes.
Add `key={row.row.oid}` to the `SummaryCell` component to provide a stable
unique identifier, matching the consistent pattern used in other column
components like Branch, CommitHash, and Timestamp in the same directory.
In `@apps/desktop/src/components/historyGraph/helper.tsx`:
- Around line 102-116: The getRemoteRefs function hardcodes filtering to only
include refs/remotes/origin/* branches, which excludes remote-tracking branches
from other remotes like upstream. Additionally, both getBranchRefs and
getRemoteRefs only filter out origin/HEAD symbolic refs, allowing other remote
symbolic HEAD refs through. Update getRemoteRefs to match any refs/remotes/*
pattern instead of just origin, and update both getBranchRefs and getRemoteRefs
to filter out all remote symbolic HEAD refs (any refs/remotes/*/HEAD pattern)
rather than just the origin variant.
In `@apps/desktop/src/components/historyGraph/PanoramaChart.tsx`:
- Around line 85-100: The applyViewport callback performs division by ws (window
size) on lines 99-100, but when totalCommits is zero, winOf returns zero,
causing division by zero and invalid CSS values. Add a guard clause at the
beginning of the applyViewport useCallback function that returns early if
totalCommits equals zero, preventing the downstream ratio calculations from
executing. Apply the same guard check to the other affected location at lines
157-159.
In `@apps/desktop/src/routes/app/inbox/index.tsx`:
- Around line 16-23: The useGitHistoryGraph hook is being initialized with an
excessively large limit value of 10,000 for the initial page, causing
performance degradation in large repositories. Reduce the limit parameter in the
useGitHistoryGraph call to a smaller, more reasonable initial page size that
balances user experience with performance, such as a value in the range of
50-100, allowing subsequent pages to be fetched as needed through pagination.
In `@apps/desktop/src/routes/app/issues/index.tsx`:
- Around line 18-25: The div container with gridTemplateColumns and scrollRef is
not configured to allow vertical scrolling despite wheel event handlers in lines
92-96 attempting to modify scrollTop. Add overflowY property set to "auto" or
"scroll" in the style object of this div element (alongside the existing
overflowX: "hidden") to enable vertical scrolling on this container, ensuring
the wheel event consumption in the handler code can actually scroll the content
vertically.
In `@packages/commands/src/types.ts`:
- Around line 206-216: The search field in the HistoryQuerySchema export is
unused throughout the service logic and does not appear in any query processing
functions such as build_cache_key, build_log_args, or build_revision_args.
Remove the search property line (z.string().optional()) from the
HistoryQuerySchema object definition. Additionally, remove the corresponding
search field from the Rust HistoryQuery struct defined in
crates/git/models/graph.rs to maintain consistency between the TypeScript and
Rust contracts.
In `@packages/ui/src/components/chart.tsx`:
- Around line 243-246: The conditional check `{item.value &&` on line 243 uses a
truthy check which treats the numeric value 0 as falsy and prevents it from
being rendered. Replace this truthy check with an explicit nullish check such as
`item.value != null` to properly allow zero values to display while still
preventing null and undefined values from rendering in the tooltip span element.
- Around line 80-99: The ChartStyle component uses dangerouslySetInnerHTML with
dynamic values including the id parameter, colorConfig keys, and color values
that are interpolated directly into CSS without escaping or validation, creating
an XSS vulnerability. Escape the id variable before using it in the
[data-chart=${id}] selector to prevent selector injection, validate and escape
each key from colorConfig before using it in the --color-${key} property name,
and validate the color value before inserting it into the CSS rule to ensure it
cannot break out of the style context. Consider using CSS variables or a
CSS-in-JS approach as a safer alternative to dangerouslySetInnerHTML.
- Line 1: Remove the `@ts-nocheck` directive at the top of the chart component
file to re-enable TypeScript type checking. Then address the type safety issues:
replace the 9 unsafe `as keyof typeof` assertions with proper type guards or
explicit type narrowing instead of type assertions, add proper type guards
before accessing unknown type properties to ensure they exist before use, and
specifically fix the potential undefined property access where
`item.value.toLocaleString()` is called by adding a null/undefined check to
ensure item.value exists and is of the correct type before invoking the
toLocaleString method.
---
Nitpick comments:
In `@apps/desktop/src/components/historyGraph/columns/branch/branch-cell.tsx`:
- Around line 17-65: The BranchCell component wrapped with React.memo does not
have a display name assigned, causing React DevTools to display it as
Memo(Component) instead of the actual component name. After the BranchCell
assignment is complete with React.memo, add a displayName property assignment to
the BranchCell variable with the value "BranchCell" to make the component
identifiable in React DevTools for easier debugging.
- Around line 114-126: The fragment wrapper around the map function in the local
branch refs rendering is unnecessary. Remove the empty fragment tags (the <>
</>) that wrap the map call, and move the map function directly into the
conditional expression where it renders the BranchBadge components for each ref
in row.localBranchRefs.
- Around line 158-159: Remove the commented-out backgroundColor property that
uses color-mix in the branch-cell styling section. The line containing the
commented code with backgroundColor and color-mix should be deleted entirely.
Additionally, remove any other commented-out code at the other location
mentioned in the review (line 179) that has similar styling comments. Clean up
all unused commented code to keep the codebase maintainable.
- Around line 127-139: The fragment wrapper around the row.remoteRefs.map call
is unnecessary since it only contains a single mapped element producing
BranchBadge components. Remove the outer fragment delimiters (the empty angle
brackets) while keeping the conditional check and the map function intact. This
simplifies the JSX structure without changing the functionality or how the
BranchBadge components are rendered with their key, ref, row, currentBranch, and
type props.
- Around line 43-59: The fragment wrapper around the BranchBadge component in
the branchRefs.map callback is unnecessary since there is only a single child
element being rendered. Remove the outer empty fragment operators `<>` and `</>`
that wrap the BranchBadge component, returning the BranchBadge component
directly from the map function instead.
- Around line 140-152: The unnecessary fragment wrapper around the
row.tags.map() call should be removed. Since the conditional check
{row.tags.length > 0 && ...} already ensures the block only renders when tags
exist, and the fragment is only wrapping a single map operation that generates
BranchBadge components, you can safely remove the <> and </> fragment delimiters
and render the map call directly within the conditional expression.
In `@apps/desktop/src/components/historyGraph/columns/branch/index.tsx`:
- Around line 6-27: The Branch component created with React.memo lacks a
displayName property, causing React DevTools to display it as "Memo(Component)"
instead of the actual component name. After the React.memo assignment for the
Branch component, add a displayName property assignment to the Branch constant
to explicitly set its display name to 'Branch'.
In
`@apps/desktop/src/components/historyGraph/columns/commiters/commiters-cell.tsx`:
- Around line 18-117: The CommitersCell component is wrapped in React.memo but
lacks a displayName property, causing React DevTools to display it as
Memo(Component) instead of the actual component name. Add a displayName property
assignment after the component export to set it to "CommitersCell", which will
help with debugging and make the component easier to identify in React DevTools.
- Around line 40-44: The initials extraction logic (splitting a name by spaces,
taking the first character of each word, joining them, and converting to
uppercase) is duplicated three times in the commiters-cell.tsx file at lines
40-44, 68-72, and 100-104. Create a reusable helper function called getInitials
that takes a name string parameter and returns the formatted initials, then
replace all three duplicate code blocks with calls to this new helper function
to eliminate the repetition.
In `@apps/desktop/src/components/historyGraph/columns/commiters/index.tsx`:
- Around line 5-13: The Commiters component is wrapped with React.memo but lacks
a displayName property, causing React DevTools to display it as
"Memo(Commiters)" instead of just "Commiters", which hampers debugging. Add a
displayName property to the Commiters component after it is defined by assigning
Commiters.displayName equal to the string "Commiters" to explicitly set how it
appears in React DevTools.
In `@apps/desktop/src/components/historyGraph/columns/stats/index.tsx`:
- Around line 5-13: The Stats component is wrapped in React.memo but lacks a
displayName property, which causes React DevTools to display it as
Memo(Component) instead of Stats, making debugging difficult. After the Stats
component definition (following the closing `});`), add a displayName property
to the Stats component by assigning it the string value "Stats" to ensure proper
identification in React DevTools.
In `@apps/desktop/src/components/historyGraph/columns/stats/stats-cell.tsx`:
- Around line 10-38: The StatsCell component is wrapped with React.memo() but
does not have a display name assigned, which causes React DevTools to show it as
Memo(Component) instead of StatsCell. Add a displayName property to the
StatsCell constant after the React.memo() declaration by assigning
StatsCell.displayName equal to the string 'StatsCell' to improve debugging
visibility in React DevTools.
In `@crates/git/models/graph.rs`:
- Around line 79-91: Add the Clone trait to the derive macro for the
CommitActivityQuery struct. Include Clone in the derive attribute alongside
Debug, Serialize, and Deserialize to allow for easy cloning of query objects
without requiring ownership transfer. Since the struct contains only primitive
types (usize and bool), Clone can be derived automatically.
In `@crates/git/service/graph.rs`:
- Around line 801-820: The head_index assignment inside the .map() closure is a
side effect that should be avoided in a mapping operation. Refactor this code to
separate the concerns: either use .enumerate().fold() to accumulate both the
CommitActivityItem collection and the head_index value in a single pass, or
extract the HEAD matching logic into a separate operation outside the map
operation (such as finding the index where the oid matches head_oid.as_deref()
after collecting the items). This will make the intent clearer and avoid
mutating external state within the transformation closure.
In `@crates/git/service/history.rs`:
- Around line 57-63: The synchronous `graph_service::commit_activity` function
performs blocking git subprocess calls within the async `commit_activity`
method, which could block the Tokio runtime thread. If async task starvation
becomes an issue in production, wrap the `graph_service::commit_activity` call
using `tokio::task::spawn_blocking` to offload the blocking work to a separate
thread pool and prevent runtime starvation. This ensures the async context is
not blocked by long-running synchronous operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc9cf612-4995-4342-ade0-4efdf50acef4
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
apps/desktop/package.jsonapps/desktop/src-tauri/src/commands/history.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src/app.cssapps/desktop/src/components/diffBoxes.tsxapps/desktop/src/components/historyGraph/PanoramaChart.tsxapps/desktop/src/components/historyGraph/body.tsxapps/desktop/src/components/historyGraph/columns/branch/branch-badge.tsxapps/desktop/src/components/historyGraph/columns/branch/branch-cell.tsxapps/desktop/src/components/historyGraph/columns/branch/index.tsxapps/desktop/src/components/historyGraph/columns/commit-hash/commit-hash-cell.tsxapps/desktop/src/components/historyGraph/columns/commit-hash/index.tsxapps/desktop/src/components/historyGraph/columns/commiters/commiters-cell.tsxapps/desktop/src/components/historyGraph/columns/commiters/index.tsxapps/desktop/src/components/historyGraph/columns/lanes/color-palettes.tsapps/desktop/src/components/historyGraph/columns/lanes/index.tsxapps/desktop/src/components/historyGraph/columns/lanes/lane-background.tsxapps/desktop/src/components/historyGraph/columns/lanes/lane-cell.tsxapps/desktop/src/components/historyGraph/columns/lanes/lane-graph.tsxapps/desktop/src/components/historyGraph/columns/stats/index.tsxapps/desktop/src/components/historyGraph/columns/stats/stats-cell.tsxapps/desktop/src/components/historyGraph/columns/summary/index.tsxapps/desktop/src/components/historyGraph/columns/summary/summary-cell.tsxapps/desktop/src/components/historyGraph/columns/timestamp/index.tsxapps/desktop/src/components/historyGraph/columns/timestamp/timestamp-cell.tsxapps/desktop/src/components/historyGraph/helper.tsxapps/desktop/src/components/historyGraph/index.tsxapps/desktop/src/components/historyGraph/virtualizer.tsapps/desktop/src/hooks/useRepository.tsapps/desktop/src/routes/app/inbox/index.tsxapps/desktop/src/routes/app/issues/index.tsxapps/desktop/src/state/domains/RepositoryState.tsapps/desktop/src/types/app.tscrates/git/models/commit.rscrates/git/models/graph.rscrates/git/models/history.rscrates/git/parsers/graph.rscrates/git/service/graph.rscrates/git/service/history.rscrates/git/tests/history_service.rspackage.jsonpackages/commands/src/.typecachepackages/commands/src/commands.tspackages/commands/src/index.tspackages/commands/src/types.tspackages/ui/package.jsonpackages/ui/src/components/avatar.tsxpackages/ui/src/components/button.tsxpackages/ui/src/components/chart.tsxpackages/ui/src/styles/base.css
💤 Files with no reviewable changes (1)
- packages/ui/src/styles/base.css
Comment on lines
+65
to
+120
| useEffect(() => { | ||
| const root = scrollRef.current; | ||
| if (!root) return; | ||
|
|
||
| let activeRowId: string | undefined | null = null; | ||
|
|
||
| const map = new Map<string, HTMLElement[]>(); | ||
|
|
||
| const getRowEls = (rowId: string) => { | ||
| let els = map.get(rowId); | ||
| if (!els) { | ||
| els = Array.from( | ||
| root.querySelectorAll(`[data-cell-id="${rowId}"]`), | ||
| ) as HTMLElement[]; | ||
| map.set(rowId, els); | ||
| } | ||
| return els; | ||
| }; | ||
|
|
||
| const apply = (rowId: string | undefined | null) => { | ||
| if (activeRowId === rowId) return; | ||
|
|
||
| if (activeRowId) { | ||
| getRowEls(activeRowId).forEach((el) => | ||
| hoverClasses.forEach((c) => el.classList.remove(c)), | ||
| ); | ||
| } | ||
|
|
||
| if (rowId) { | ||
| getRowEls(rowId).forEach((el) => | ||
| hoverClasses.forEach((c) => el.classList.add(c)), | ||
| ); | ||
| } | ||
|
|
||
| activeRowId = rowId; | ||
| }; | ||
|
|
||
| const onMove = (e: PointerEvent) => { | ||
| const el = (e.target as Element)?.closest?.("[data-cell]"); | ||
| if (!el) { | ||
| apply(null); | ||
| return; | ||
| } | ||
| apply((el as HTMLElement).dataset.cellId); | ||
| }; | ||
|
|
||
| const onLeave = () => apply(null); | ||
|
|
||
| root.addEventListener("pointerover", onMove); | ||
| root.addEventListener("pointerleave", onLeave); | ||
|
|
||
| return () => { | ||
| root.removeEventListener("pointerover", onMove); | ||
| root.removeEventListener("pointerleave", onLeave); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
Effect missing hoverClasses dependency causes stale closure.
The effect closure captures hoverClasses but doesn't list it in the dependency array, so if hoverClasses changes, the event handlers will continue using the stale array.
🐛 Proposed fix
return () => {
root.removeEventListener("pointerover", onMove);
root.removeEventListener("pointerleave", onLeave);
};
- }, []);
+ }, [hoverClasses]);Note: Since hoverClasses is redeclared on every render (lines 43-46), this will recreate the effect on every render. Consider moving hoverClasses outside the component or memoizing it:
+const HOVER_CLASSES = [
+ cn("bg-secondary/70"),
+ cn("[&_[data-hidden-branch-refs]]:flex"),
+];
+
const GraphBody = ({
rows,
fetchNextPage,
hasNextPage,
isFetchingNextPage,
scrollRef,
}: GraphBodyProps) => {
- const hoverClasses = [
- cn("bg-secondary/70"),
- cn("[&_[data-hidden-branch-refs]]:flex"),
- ];Then use HOVER_CLASSES in the effect.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/components/historyGraph/body.tsx` around lines 65 - 120, The
useEffect hook is missing hoverClasses from its dependency array, causing the
event handlers in onMove to use stale references if hoverClasses changes. Since
hoverClasses is redeclared on every render (lines 43-46), the best fix is to
move the hoverClasses constant outside the component body so it's not recreated
on each render, then reference it in the apply function within the useEffect.
Alternatively, if hoverClasses must remain in the component, add it to the
useEffect dependency array at the end, but this will cause the effect to
recreate on every render due to the constant being redeclared.
Comment on lines
+71
to
+82
| const map = new Map<string, HTMLElement[]>(); | ||
|
|
||
| const getRowEls = (rowId: string) => { | ||
| let els = map.get(rowId); | ||
| if (!els) { | ||
| els = Array.from( | ||
| root.querySelectorAll(`[data-cell-id="${rowId}"]`), | ||
| ) as HTMLElement[]; | ||
| map.set(rowId, els); | ||
| } | ||
| return els; | ||
| }; |
There was a problem hiding this comment.
Map cache grows unbounded as rows change.
The Map caches DOM query results but is never cleared when rows change, causing memory leaks as stale row IDs accumulate.
🛡️ Proposed fix
Clear the map when rows change or when the effect re-runs:
useEffect(() => {
const root = scrollRef.current;
if (!root) return;
let activeRowId: string | undefined | null = null;
const map = new Map<string, HTMLElement[]>();
const getRowEls = (rowId: string) => {
let els = map.get(rowId);
if (!els) {
els = Array.from(
root.querySelectorAll(`[data-cell-id="${rowId}"]`),
) as HTMLElement[];
map.set(rowId, els);
}
return els;
};
// ... rest of effect
return () => {
+ map.clear();
root.removeEventListener("pointerover", onMove);
root.removeEventListener("pointerleave", onLeave);
};
}, [hoverClasses]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const map = new Map<string, HTMLElement[]>(); | |
| const getRowEls = (rowId: string) => { | |
| let els = map.get(rowId); | |
| if (!els) { | |
| els = Array.from( | |
| root.querySelectorAll(`[data-cell-id="${rowId}"]`), | |
| ) as HTMLElement[]; | |
| map.set(rowId, els); | |
| } | |
| return els; | |
| }; | |
| useEffect(() => { | |
| const root = scrollRef.current; | |
| if (!root) return; | |
| let activeRowId: string | undefined | null = null; | |
| const map = new Map<string, HTMLElement[]>(); | |
| const getRowEls = (rowId: string) => { | |
| let els = map.get(rowId); | |
| if (!els) { | |
| els = Array.from( | |
| root.querySelectorAll(`[data-cell-id="${rowId}"]`), | |
| ) as HTMLElement[]; | |
| map.set(rowId, els); | |
| } | |
| return els; | |
| }; | |
| // ... rest of effect | |
| return () => { | |
| map.clear(); | |
| root.removeEventListener("pointerover", onMove); | |
| root.removeEventListener("pointerleave", onLeave); | |
| }; | |
| }, [hoverClasses]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/components/historyGraph/body.tsx` around lines 71 - 82, The
`map` variable caches DOM query results indefinitely without clearing stale
entries when `rows` change, causing unbounded memory growth. Add a useEffect
hook that depends on `rows` and clears the `map` cache by calling `map.clear()`
whenever `rows` changes, ensuring the cache doesn't accumulate stale row IDs and
old DOM references.
Comment on lines
+142
to
+150
| {columns.map(({ Component }, idx) => ( | ||
| <Component | ||
| key={idx} | ||
| rows={processedRows} | ||
| style={getColumnStyle(idx + 1, TOTAL_ROWS)} | ||
| layout={layout} | ||
| scrollRef={scrollRef} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Use stable keys for column components.
Array index as key can cause React to incorrectly reuse component instances if the columns array order changes.
🐛 Proposed fix
{columns.map(({ Component }, idx) => (
<Component
- key={idx}
+ key={columns[idx].id}
rows={processedRows}
style={getColumnStyle(idx + 1, TOTAL_ROWS)}
layout={layout}
scrollRef={scrollRef}
/>
))}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/components/historyGraph/body.tsx` around lines 142 - 150,
The columns.map() function is using the array index idx as the React key, which
can cause component reuse issues if the columns array order changes. Replace the
key={idx} with a stable identifier from the column object itself, such as a
unique id property or column name that remains constant regardless of array
position. If the column objects don't have a unique identifier, consider adding
one or using a property that uniquely identifies each column component.
| <Avatar className="ring-2 ring-background rounded-sm size-4.5"> | ||
| <AvatarImage | ||
| alt={commitRow.commit?.authors.author.name} | ||
| src={`https://avatars.githubusercontent.com/u/e?email=${commitRow.commit?.authors.author.email}&s=64`} |
There was a problem hiding this comment.
Email addresses exposed in avatar URLs may leak PII.
GitHub avatar URLs contain email addresses in query parameters, which may be logged by proxies, CDNs, or browser history, potentially violating GDPR/CCPA. Consider hashing emails or using GitHub user IDs if available.
Also applies to: 65-65, 97-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/components/historyGraph/columns/commiters/commiters-cell.tsx`
at line 37, The avatar URL in the commiters-cell component is exposing email
addresses as query parameters, which creates a PII leak risk that can be logged
by proxies, CDNs, or browser history. Replace the email query parameter in the
src attribute with either a hashed version of the email or a GitHub user ID if
available from the commitRow data. Apply this same change to all occurrences
mentioned (lines 37, 65, and 97) where the avatar URL is constructed with the
email parameter in the query string.
Comment on lines
+18
to
+25
| <div | ||
| style={{ | ||
| display: "grid", | ||
| gridTemplateColumns: "minmax(min-content, max-content) 400px 1fr", | ||
| overflowX: "hidden", | ||
| width: "100%", | ||
| }} | ||
| ref={scrollRef} |
There was a problem hiding this comment.
Vertical wheel events are redirected to a non-scrollable element.
The container referenced at Line 25 is not set up for vertical scrolling, but Lines 92–96 consume wheel events and write scrollTop. With PageLayout using hidden overflow, this can block normal vertical navigation.
🛠️ Suggested fix
<div
+ className="h-full min-h-0 overflow-y-auto"
style={{
display: "grid",
gridTemplateColumns: "minmax(min-content, max-content) 400px 1fr",
overflowX: "hidden",
width: "100%",
}}
ref={scrollRef}
>
...
onWheel={(e: React.WheelEvent<HTMLDivElement>) => {
const parent = scrollRef.current;
if (!parent) return;
const isVerticalIntent = Math.abs(e.deltaY) > Math.abs(e.deltaX);
- if (isVerticalIntent) {
+ const canScrollDown =
+ parent.scrollTop + parent.clientHeight < parent.scrollHeight;
+ const canScrollUp = parent.scrollTop > 0;
+
+ if (
+ isVerticalIntent &&
+ ((e.deltaY > 0 && canScrollDown) || (e.deltaY < 0 && canScrollUp))
+ ) {
parent.scrollTop += e.deltaY;
e.preventDefault();
e.stopPropagation(); // <-- important in WKWebView
}
}}Also applies to: 86-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/routes/app/issues/index.tsx` around lines 18 - 25, The div
container with gridTemplateColumns and scrollRef is not configured to allow
vertical scrolling despite wheel event handlers in lines 92-96 attempting to
modify scrollTop. Add overflowY property set to "auto" or "scroll" in the style
object of this div element (alongside the existing overflowX: "hidden") to
enable vertical scrolling on this container, ensuring the wheel event
consumption in the handler code can actually scroll the content vertically.
ruru-m07
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores