Skip to content

feat(cell-action): add external linking as action #95892

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

Merged
merged 7 commits into from
Jul 25, 2025

Conversation

lzhao-sentry
Copy link
Contributor

@lzhao-sentry lzhao-sentry commented Jul 18, 2025

Changes

Have external linking as a dropdown menu option for CellAction. Removed cell actions for some ID linked fields (since it conflicts with the dropdown & should just directly open the link)

Video

external.linking.mov

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jul 18, 2025
@lzhao-sentry lzhao-sentry marked this pull request as ready for review July 21, 2025 13:36
@lzhao-sentry lzhao-sentry requested review from a team as code owners July 21, 2025 13:36
cursor[bot]

This comment was marked as outdated.

Copy link
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

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

👀 ! Two questions:

  1. In a few places the isUrl functionality is checking the feature flag, but sometimes not, is that intentional?
  2. I'm worried that people won't remember to import handleCellActionFallback, and this'll result in inconsistent fallback handling. Is it possible to handle the fallback inside CellActions?

cursor[bot]

This comment was marked as outdated.

@lzhao-sentry
Copy link
Contributor Author

  1. I'm worried that people won't remember to import handleCellActionFallback, and this'll result in inconsistent fallback handling. Is it possible to handle the fallback inside CellActions?

@gggritso I think we would need to refactor how handleCellAction works since there's no consistent way to know if an action was successfully handled. We could add a return type, but I feel like that puts more work on the user.

Would it be better to split it into two helper functions (e.g., copyToClipboard and openExternalLink)? I know it turns it into two imports, but I think the naming makes it clear the purpose of the functions?

@gggritso
Copy link
Member

@lzhao-sentry I like that idea! Go for it 👍🏻

cursor[bot]

This comment was marked as resolved.

Copy link
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

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

👍🏻

@lzhao-sentry
Copy link
Contributor Author

@gggritso Apologies, need a re-review. Was showing this to Dora and Dhrumil and they asked for logs to still use the ellipsis trigger since the bold hover trigger doesn't fit well there. I added an extra prop and enum to support this

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: Clipboard Copy Triggers Unnecessary Query Update

The Actions.COPY_TO_CLIPBOARD case was removed from the handleCellAction switch statement. Copy actions now fall through to the default case, which calls updateQuery. This unnecessarily executes query modification logic and browser history navigation, despite updateQuery handling the copy internally.

static/app/views/discover/table/tableView.tsx#L611-L625

}
default: {
// Some custom perf metrics have units.
// These custom perf metrics need to be adjusted to the correct value.
let cellValue = value;
const unit = tableData?.meta?.units?.[column.name];
if (typeof cellValue === 'number' && unit) {
if (Object.keys(SIZE_UNITS).includes(unit)) {
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message
cellValue *= SIZE_UNITS[unit];
} else if (Object.keys(DURATION_UNITS).includes(unit)) {
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message
cellValue *= DURATION_UNITS[unit];
}
}

Fix in CursorFix in Web


const cellActions = makeCellActions(props);
const {children, column} = props;

const useCellActionsV2 = organization.features.includes('discover-cell-actions-v2');
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Feature Flag Naming Error

The discover-cell-actions-v2 feature flag check is missing the organizations: prefix, it should be organizations:discover-cell-actions-v2.

Locations (4)

Fix in CursorFix in Web

Copy link
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

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

Logs is the only table to use the ellipsis trigger.

:cryblood:

@lzhao-sentry lzhao-sentry merged commit 1727034 into master Jul 25, 2025
46 checks passed
@lzhao-sentry lzhao-sentry deleted the lzhao/feat/add-external-linking-to-cell-actions branch July 25, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants