Skip to content

Display Commit Hash in ChatMessage UI#3191

Open
nourzakhama2003 wants to merge 11 commits intodyad-sh:mainfrom
nourzakhama2003:feat/version-id-chat-indicator
Open

Display Commit Hash in ChatMessage UI#3191
nourzakhama2003 wants to merge 11 commits intodyad-sh:mainfrom
nourzakhama2003:feat/version-id-chat-indicator

Conversation

@nourzakhama2003
Copy link
Copy Markdown
Collaborator

@nourzakhama2003 nourzakhama2003 commented Apr 10, 2026

closes #2416
This PR addresses the following improvements and fixes in the chat message UI for assistant messages:

Commit Hash Display: The commit hash is now shown as a sibling to the commit message in the metadata row, following the commit message and icon.
Minimal Tooltip: The tooltip for the commit hash now displays only the commit hash itself (no commit message or extra text), providing a clean and focused UX.
Copy-to-Clipboard: Users can copy the full commit hash by clicking the hash or copy icon
image


Open with Devin

@wwwillchen
Copy link
Copy Markdown
Collaborator

@BugBot run

gemini-code-assist[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

cubic-dev-ai[bot]

This comment was marked as resolved.

dyad-assistant[bot]

This comment was marked as resolved.

@dyad-assistant
Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: β›” NO - Do NOT merge

Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard.

Issues Summary

Severity File Issue
πŸ”΄ HIGH src/components/chat/ChatMessage.tsx:296 Broken regex no longer strips [dyad] prefix from commit messages
🟑 MEDIUM src/components/chat/ChatMessage.tsx:292 Title tooltip shows commit hash instead of full commit message
🟑 MEDIUM src/components/chat/ChatMessage.tsx:308 Clipboard copy lacks error handling and visual feedback
🟒 Low Priority Notes (1 item)
  • Redundant typeof checks - src/components/chat/ChatMessage.tsx:307 β€” typeof message.commitHash === "string" checks are unnecessary inside the {message.commitHash && (...)} guard which already narrows the type.
🚫 Dropped False Positives (0 items)

All flagged issues were confirmed as valid.


Generated by Dyadbot multi-agent code review

@nourzakhama2003 nourzakhama2003 changed the title Display Commit Hash with Minimal Tooltip in ChatMessage UI Display Commit Hash in ChatMessage UI Apr 10, 2026
@wwwillchen
Copy link
Copy Markdown
Collaborator

@BugBot run

devin-ai-integration[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@dyad-assistant
Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: πŸ€” NOT SURE - Potential issues

Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard.

Issues Summary

Severity File Issue
🟑 MEDIUM src/components/chat/ChatMessage.tsx:306 Clipboard copy lacks error handling and copy-success feedback (inconsistent with Request ID pattern)
🟑 MEDIUM src/components/chat/ChatMessage.tsx:307 Redundant typeof checks inside truthy commitHash guard
🟒 Low Priority Notes (2 items)
  • Redundant typeof checks - src/components/chat/ChatMessage.tsx:307 β€” After the message.commitHash && guard, typeof message.commitHash === "string" is always true. The ternary else branches returning "" are dead code.
  • Tooltip lacks action context - src/components/chat/ChatMessage.tsx:323 β€” The tooltip shows only the raw hash. The Request ID tooltip shows "Copy Request ID: abc123..." β€” consider matching that pattern with "Copy commit hash: abc1234...".
🚫 Dropped False Positives (2 items)
  • TooltipTrigger renders empty self-closing button (flagged by 2 agents as HIGH) β€” Dropped: Ariakit's render prop composes children into the rendered element. The Request ID button at lines ~334-368 uses the exact same render={<button ... />} + children pattern and works correctly in production.
  • Commit hash hidden when no messageVersion.message (flagged by 2 agents as MEDIUM) β€” Dropped: commitHash and messageVersion.message both originate from the same commit operation. The nesting is semantically correct since the commit hash is part of the commit info section.

Generated by Dyadbot multi-agent code review

dyad-assistant[bot]

This comment was marked as resolved.

@github-actions github-actions bot added the needs-human:review-issue ai agent flagged an issue that requires human review label Apr 10, 2026
@wwwillchen
Copy link
Copy Markdown
Collaborator

@BugBot run

devin-ai-integration[bot]

This comment was marked as resolved.

cubic-dev-ai[bot]

This comment was marked as resolved.

@dyad-assistant
Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: πŸ€” NOT SURE - Potential issues

Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard.

Issues Summary

Severity File Issue
🟑 MEDIUM src/components/chat/ChatMessage.tsx:134 copiedCommitHashTimeoutRef not cleaned up on unmount
🟒 Low Priority Notes (3 items)
  • Leftover personal note in catch block - src/components/chat/ChatMessage.tsx:372 β€” The comment //i can display toast error reads like a developer note, not a descriptive comment. Use // Silently ignore clipboard errors or a proper // TODO: if it's planned work.
  • Static aria-label doesn't reflect copied state - src/components/chat/ChatMessage.tsx:331 β€” The button's aria-label is always "Copy Commit Hash" even after copy succeeds. Screen reader users won't know the action succeeded. Consider aria-label={copiedCommitHash ? 'Commit hash copied' : 'Copy commit hash'}.
  • Copy icon placement inconsistent with Request ID - src/components/chat/ChatMessage.tsx:336 β€” Commit hash shows text-then-icon, while Request ID shows icon-then-text. Minor visual inconsistency in the metadata row.
🚫 Dropped False Positives (2 items)
  • Duplicated copy-to-clipboard logic β€” Dropped: The PR follows the existing Request ID copy pattern already in the file. This is a pre-existing concern, not new tech debt introduced by this PR.
  • Tooltip shows full 40-char hash β€” Dropped: This is standard git UX (short hash visible, full hash on hover for copying). Intentional per PR description.

Generated by Dyadbot multi-agent code review

dyad-assistant[bot]

This comment was marked as resolved.

@wwwillchen
Copy link
Copy Markdown
Collaborator

@BugBot run

dyad-assistant[bot]

This comment was marked as resolved.

@wwwillchen
Copy link
Copy Markdown
Collaborator

@BugBot run

@dyad-assistant
Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: βœ… YES - Ready to merge

Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard.

Issues Summary

Severity File Issue
🟑 MEDIUM src/hooks/useCopyToClipboard.ts:53 copyRawText duplicates clipboard + state boilerplate
🟑 MEDIUM src/components/chat/ChatMessage.tsx:294 Commit hash has no resting-state interactivity affordance
🟒 Low Priority Notes (3 items)
  • Dead comment - src/components/chat/ChatMessage.tsx:127 β€” // handle copy request id and commit hash using CopyButton is a leftover stub with no associated code below it
  • raw=false default never used - src/components/ui/copy-button.tsx:24 β€” Both call sites pass raw={true}, making the copyMessageContent branch dead code. Consider flipping the default or removing the prop
  • Missing type="button" - src/components/ui/copy-button.tsx:41 β€” The <button> defaults to type="submit" in HTML; adding type="button" is a best practice
🚫 Dropped False Positives (6 items)
  • Shared copied state fragility β€” Dropped: theoretical concern only; raw prop is fixed at construction time so both copy paths are never called on the same instance
  • GitHub test snapshot no longer verifies push success β€” Dropped: already commented on in prior review rounds
  • Commit hash gated on messageVersion β€” Dropped: already commented on in prior review rounds
  • Mono font / text swaps to "Copied" β€” Dropped: already commented on in prior review rounds
  • if (\!value) return guard is misleading β€” Dropped: harmless defensive check for empty string; too nitpicky
  • flex-wrap + space-x-2 wrapping bug β€” Dropped: pre-existing pattern not introduced by this PR

Generated by Dyadbot multi-agent code review

dyad-assistant[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator

@wwwillchen wwwillchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nourzakhama2003 thanks for the PR - I don't think we should show the commit hash. the user is asking to see the version number. right now we order the commit hashes from 1,2,3, etc. and show that in the version list so the user wants to see the version number as well in the chat message.

@wwwillchen
Copy link
Copy Markdown
Collaborator

@BugBot run

@wwwillchen
Copy link
Copy Markdown
Collaborator

@BugBot run

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’‘ Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 962751bdaf

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 πŸ‘.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/components/chat/ChatMessage.tsx
Copy link
Copy Markdown
Contributor

@dyad-assistant dyad-assistant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-agent review: 3 MEDIUM issues found (no HIGH blockers)

Comment thread src/components/chat/versionUtils.ts
Comment thread src/components/ui/copy-button.tsx
Comment thread src/components/ui/copy-button.tsx
@dyad-assistant
Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: πŸ€” NOT SURE - Potential issues

Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard.

No HIGH-severity blockers. Three MEDIUM issues worth addressing before merge β€” see inline comments for details.

Issues Summary

Severity File Issue
🟑 MEDIUM src/components/chat/versionUtils.ts:7 calculateVersionNumber uses indexOf (reference equality), contradicting its { oid: string } signature
🟑 MEDIUM src/components/ui/copy-button.tsx:43 aria-label doesn't contain visible text (WCAG 2.5.3); no announcement on copy success
🟑 MEDIUM src/components/ui/copy-button.tsx:57 Version button tooltip duplicates its own visible label instead of describing the action
🟒 Low Priority Notes (6 items)
  • Stale orphan comment β€” src/components/chat/ChatMessage.tsx:133 β€” // handle copy request id and commit hash using CopyButton no longer introduces any logic (code moved into CopyButton); it's a WHAT comment with nothing to narrate.
  • Trivial file-level comment β€” src/components/chat/versionUtils.ts:1 β€” //Calculate the version number from a version object using reverse index. restates the signature. A short JSDoc explaining the ordering invariant (versions[] is oldestβ†’newest, UI labels newest as Version 1) would explain the why instead.
  • PR title / implementation drift β€” PR title says "Display Commit Hash" but the UI actually surfaces and copies Version N, not the commit hash. Worth aligning the PR title (and eventual merge-commit message) with what ships.
  • Request ID tooltip unconditional ellipsis β€” src/components/chat/ChatMessage.tsx:319 β€” requestId.slice(0, 8) + "..." appends ... even if the ID is shorter than 8 chars. Cosmetic only; tooltip has room for the full value.
  • Mixed tooltip systems on adjacent elements β€” commit-message span uses native title= (OS-styled, delayed) while the sibling Version button uses the custom Tooltip component (themed, instant). Minor polish β€” visually inconsistent within ~20px.
  • Label swap layout shift β€” when the Version button's text flips from Version 3 to Copied, the different widths cause a small horizontal shift that nudges the adjacent Request ID button. A min-width on the label span or keeping the label stable (swapping only the icon) would fix it.
🚫 Dropped False Positives (3 items)
  • Dead imports in ChatMessage.tsx β€” Dropped: Tooltip/TooltipTrigger/TooltipContent are still used at ChatMessage.tsx:210-231, and useCopyToClipboard at ChatMessage.tsx:104. None are actually unused.
  • Version button disappears for historical messages without a matching live version β€” Dropped: the Version button lives inside the {messageVersion && messageVersion.message && (...)} block, so when messageVersion is null, the commit message row and the version button both hide together β€” internally consistent, not a regression.
  • versionNumber === 0 would suppress the button β€” Dropped: calculateVersionNumber cannot return 0 in practice (smallest legitimate value is 1 when the message is the latest version; not-found returns length + 1). The {versionNumber && ...} guard is fine.

Generated by Dyadbot multi-agent code review

@nourzakhama2003
Copy link
Copy Markdown
Collaborator Author

nourzakhama2003 commented Apr 14, 2026

@nourzakhama2003 thanks for the PR - I don't think we should show the commit hash. the user is asking to see the version number. right now we order the commit hashes from 1,2,3, etc. and show that in the version list so the user wants to see the version number as well in the chat message.

thanks @wwwillchen for the review , I changed the commit hash display to version number to show the version number instead of commit hash in chat messages. The other issues flagged are pre-existing code I only refactored, not changes I introduced.

@github-actions
Copy link
Copy Markdown
Contributor

🎭 Playwright Test Results

❌ Some tests failed

OS Passed Failed Flaky Skipped
🍎 macOS 422 7 4 134
πŸͺŸ Windows 426 7 8 134

Summary: 848 passed, 14 failed, 12 flaky, 268 skipped

Failed Tests

🍎 macOS

  • queued_message.spec.ts > editing queued message restores attachments and selected components
    • Error: expect(locator).toBeVisible() failed
  • queued_message.spec.ts > canceling queued message edit clears restored components
    • Error: expect(locator).toBeVisible() failed
  • socket_firewall.spec.ts > build mode - blocked unsafe npm package shows the real socket verdict and preserves app files
    • Error: expect(locator).toBeVisible() failed
  • switch_versions.spec.ts > switch versions (native git)
    • Error: locator.textContent: Error: strict mode violation: getByRole('button', { name: 'Version' }) resolved to 2 elements:
  • switch_versions.spec.ts > switch versions (isomorphic git)
    • Error: locator.textContent: Error: strict mode violation: getByRole('button', { name: 'Version' }) resolved to 2 elements:
  • version_search.spec.ts > version search
    • Error: expect(locator).toHaveText(expected) failed
  • visual_editing.spec.ts > swap image via URL
    • TimeoutError: locator.click: Timeout 30000ms exceeded.

πŸͺŸ Windows

  • chat_tabs.spec.ts > tabs appear after navigating between chats
    • Error: expect(locator).toBeVisible() failed
  • chat_tabs.spec.ts > closing a tab removes it and selects adjacent tab
    • TimeoutError: locator.click: Timeout 30000ms exceeded.
  • chat_tabs.spec.ts > only shows tabs for chats opened in current session
    • Error: expect(locator).toBeVisible() failed
  • edit_code.spec.ts > edit code edits the right file during rapid switches
    • Error: expect(received).toEqual(expected) // deep equality
  • github.spec.ts > create and sync to new repo
    • Error: expect(locator).toHaveClass(expected) failed
  • github.spec.ts > create and sync to existing repo
    • Error: expect(locator).toMatchAriaSnapshot(expected) failed
  • github.spec.ts > create and sync to existing repo - custom branch
    • Error: expect(locator).toMatchAriaSnapshot(expected) failed

πŸ“‹ Re-run Failing Tests (macOS)

Copy and paste to re-run all failing spec files locally:

npm run e2e \
  e2e-tests/queued_message.spec.ts \
  e2e-tests/socket_firewall.spec.ts \
  e2e-tests/switch_versions.spec.ts \
  e2e-tests/version_search.spec.ts \
  e2e-tests/visual_editing.spec.ts

⚠️ Flaky Tests

🍎 macOS

  • chat_mode.spec.ts > chat mode selector - default build mode (passed after 1 retry)
  • context_manage.spec.ts > manage context - smart context (passed after 1 retry)
  • setup_flow.spec.ts > Setup Flow > setup banner shows correct state when node.js is installed (passed after 1 retry)
  • smart_context_balanced.spec.ts > smart context balanced - simple (passed after 1 retry)

πŸͺŸ Windows

  • chat_input.spec.ts > send button disabled during pending proposal - reject (passed after 1 retry)
  • chat_tabs.spec.ts > right-click context menu: Close other tabs (passed after 1 retry)
  • chat_tabs.spec.ts > right-click context menu: Close tabs to the right (passed after 2 retries)
  • edit_code.spec.ts > edit code (passed after 2 retries)
  • per_chat_input.spec.ts > chat input is preserved when switching between chats (passed after 2 retries)
  • per_chat_input.spec.ts > input preserved when switching back and forth multiple times (passed after 1 retry)
  • reject.spec.ts > reject (passed after 1 retry)
  • setup_flow.spec.ts > Setup Flow > setup banner shows correct state when node.js is installed (passed after 1 retry)

πŸ“Š View full report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human:review-issue ai agent flagged an issue that requires human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tag the chat message with a version ID

2 participants