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.
feat(agents): copy-to-clipboard session ID pill button in agents table #454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
feat(agents): copy-to-clipboard session ID pill button in agents table #454
Changes from all commits
dd9cc2f83f4b885bc7d01f8cdc68482576cFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copySessionIdreferencessetCopiedSessionId, butAgentsTabdoes not define acopiedSessionIdstate (and this helper is not used anywhere in the file). This will throw if called and is likely leftover/unfinished; either add the missinguseState+ UI usage, or remove this block fromAgentsTab.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copySessionIdcallsnavigator.clipboard.writeTextwithout checking that the Clipboard API is available. In unsupported/insecure contexts this can throw synchronously (bypassing the Promise.catch) and will also leavedata-copiedstuck. Add anavigator?.clipboard?.writeTextguard (consistent with other clipboard code in this file) and resetcopiedSessionId/ toast appropriately when unavailable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The session-id pill is rendered as a native
<button>inside an MUI<Button>(which renders as a<button>by default). Nested buttons are invalid HTML and browsers will implicitly close the outer button, breaking click/keyboard behavior and accessibility. Refactor so the row container is not a<button>when it contains this pill (e.g., make the row a<div>/ListItemButtonwith appropriate keyboard handling, or move copy handling onto a non-nested element).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion line has a syntax error: the string literal is not closed due to an extra backslash/quote (
...${sessionId}}"). This will prevent the test file from parsing/running. Fix the string so it’s a valid JS literal and matches the intended source substring.Check failure on line 3 in tests/ui-agents-session-pill.test.mjs
tests/ui-agents-session-pill.test.mjs
Check failure on line 3 in tests/ui-agents-session-pill.test.mjs
tests/ui-agents-session-pill.test.mjs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test overwrites
globalThis.navigatorbut never restores the original value, which can leak into other tests. Capture the priornavigatorand restore it inafterEach, or usevi.stubGlobal('navigator', ...)/vi.unstubAllGlobals()to keep test isolation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copySessionIdreferencessetCopiedSessionId, butAgentsTabdoes not define acopiedSessionIdstate (and this helper is not used anywhere in the file). This will throw if called and is likely leftover/unfinished; either add the missinguseState+ UI usage, or remove this block fromAgentsTab.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copySessionIdcallsnavigator.clipboard.writeTextwithout checking that the Clipboard API is available. In unsupported/insecure contexts this can throw synchronously (bypassing the Promise.catch) and will also leavedata-copiedstuck. Add anavigator?.clipboard?.writeTextguard (consistent with other clipboard code in this file) and resetcopiedSessionId/ toast appropriately when unavailable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The session-id pill is rendered as a native
<button>inside an MUI<Button>(which renders as a<button>by default). Nested buttons are invalid HTML and browsers will implicitly close the outer button, breaking click/keyboard behavior and accessibility. Refactor so the row container is not a<button>when it contains this pill (e.g., make the row a<div>/ListItemButtonwith appropriate keyboard handling, or move copy handling onto a non-nested element).Uh oh!
There was an error while loading. Please reload this page.