Page MenuHomePhabricator

bengotow (Ben Gotow)
User

Projects

User does not belong to any projects.

User Details

User Since
May 9 2019, 3:45 PM (81 w, 2 d)

Recent Activity

Wed, Nov 25

bengotow requested review of D5292: Fix "View Asset Dashboard" link using href={} for a local app route.
Wed, Nov 25, 10:04 PM
bengotow added a comment to D5304: Fix undo functionatlity in Dagit playground.

This patch package is pretty intriguing. I'm OK with doing this in this isolated instance because we're likely to abandon codemirror at some point in the future (for Monaco) and forking / fixing the library to make this change seems like overkill. I'm curious to get @dish 's thoughts + approval on this though because he investigated that change.

Wed, Nov 25, 9:45 PM

Tue, Nov 24

bengotow accepted D5204: [dagit] Put SolidsRoot under workspace nav.

This looks good to me! I feel like it's a bit wonky to have the nice big solids list on the page, and the minimal solids list in the sidebar, but we can probably address that when we revisit that whole sidebar.

Tue, Nov 24, 7:25 PM
bengotow accepted D5209: [dagit] Add webpack analysis.
Tue, Nov 24, 7:23 PM
bengotow accepted D5211: [dagit] Update highlight.js.

This looks great to me.

Tue, Nov 24, 7:20 PM

Thu, Nov 19

bengotow closed D5147: Asset View Improvements.
Thu, Nov 19, 5:03 AM
bengotow committed R1:e01f57af131f: Asset View Improvements (authored by bengotow).
Asset View Improvements
Thu, Nov 19, 5:03 AM
bengotow updated the diff for D5147: Asset View Improvements.
  • Fix retina rendering of the sparklines
  • Expose stepStats for related step, add elapsed time to charts / matrix
Thu, Nov 19, 4:36 AM

Tue, Nov 17

bengotow added a comment to D5147: Asset View Improvements.

for whatever reason getting this error when running locally

Tue, Nov 17, 5:34 PM
bengotow closed D5146: Add a ?timestamp= option that allows you to jump to a specific line of run logs.
Tue, Nov 17, 5:30 PM
bengotow committed R1:39439f669099: Add a ?timestamp= option that allows you to jump to a specific line of run logs (authored by bengotow).
Add a ?timestamp= option that allows you to jump to a specific line of run logs
Tue, Nov 17, 5:30 PM
bengotow closed D5145: Add a hook that simulates React.useState but syncs with querystring.
Tue, Nov 17, 5:29 PM
bengotow committed R1:bea7b38e48c8: Add a hook that simulates React.useState but syncs with querystring (authored by bengotow).
Add a hook that simulates React.useState but syncs with querystring
Tue, Nov 17, 5:28 PM
bengotow updated the diff for D5145: Add a hook that simulates React.useState but syncs with querystring.

Make it robust to users accidentally capturin the setter in scopes

Tue, Nov 17, 2:52 AM
bengotow updated the diff for D5145: Add a hook that simulates React.useState but syncs with querystring.

Add tests!

Tue, Nov 17, 2:32 AM

Mon, Nov 16

bengotow accepted D5159: Consolidate some body-level CSS.

This looks good to me! I'd be surprised if the top level color makes a difference in many places? I think all the blueprint input components should have higher-specificity color styles (I hope so, because there's a dark mode 😅) but it'll be nice to soften any hard blacks we have laying around!

Mon, Nov 16, 11:00 PM
bengotow planned changes to D5145: Add a hook that simulates React.useState but syncs with querystring.
Mon, Nov 16, 10:53 PM
bengotow accepted D5157: Add justify-content to Box.

Looks good to me 👍

Mon, Nov 16, 10:47 PM

Sun, Nov 15

bengotow updated the diff for D5147: Asset View Improvements.

Self-review, split components and rm GraphQL fragments that don't map to components

Sun, Nov 15, 7:32 AM
bengotow requested review of D5147: Asset View Improvements.
Sun, Nov 15, 7:26 AM
bengotow requested review of D5146: Add a ?timestamp= option that allows you to jump to a specific line of run logs.
Sun, Nov 15, 7:11 AM
bengotow updated the summary of D5145: Add a hook that simulates React.useState but syncs with querystring.
Sun, Nov 15, 7:05 AM
bengotow requested review of D5145: Add a hook that simulates React.useState but syncs with querystring.
Sun, Nov 15, 7:03 AM

Fri, Nov 13

bengotow added a comment to D5137: [dagit] Use lint to enforce query inclusion of `id` field.

Oh this is cool - I didn't realize that there was a linter plugin for this.

Fri, Nov 13, 6:54 PM

Thu, Nov 12

bengotow accepted D5116: [dagit] Consolidate repository queries.

This looks good to me! Just added a few comments inline.

Thu, Nov 12, 9:21 PM

Mon, Nov 9

bengotow accepted D5063: [dagit] Core UI components.

Hey! I think this looks good. In the past I've been broadly skeptical of base components like this because I think learning how the custom DSL maps to CSS and then mentally unfolding it as you read the JSX isn't worth the cleanliness points of not writing style={display: flex, flexDirection: column} inline.

Mon, Nov 9, 5:31 PM
bengotow closed D5018: Periodically reload pending and recent runs in the partition matrix, unify filters.
Mon, Nov 9, 5:24 PM
bengotow committed R1:692c4fa29d71: Periodically reload pending and recent runs in the partition matrix, unify… (authored by bengotow).
Periodically reload pending and recent runs in the partition matrix, unify…
Mon, Nov 9, 5:24 PM
bengotow updated the diff for D5018: Periodically reload pending and recent runs in the partition matrix, unify filters.
  • Make sure that the component unmounting stops the last run of usePartitionQuery
Mon, Nov 9, 5:06 PM
bengotow added inline comments to D5018: Periodically reload pending and recent runs in the partition matrix, unify filters.
Mon, Nov 9, 4:56 PM
bengotow updated subscribers of D5018: Periodically reload pending and recent runs in the partition matrix, unify filters.

Hey Sandy thanks for the feedback! I fixed a few of the issues you noticed but a few other things:

Mon, Nov 9, 4:53 PM
bengotow updated the diff for D5018: Periodically reload pending and recent runs in the partition matrix, unify filters.
  • Compact the top bar of the run matrix a bit
  • Fix issue with matrix “showing data from runs outside the filter” when refreshing
Mon, Nov 9, 4:33 PM

Wed, Nov 4

bengotow requested review of D5018: Periodically reload pending and recent runs in the partition matrix, unify filters.
Wed, Nov 4, 4:49 PM

Tue, Nov 3

bengotow closed D5005: Second round of improvements to partition UI.
Tue, Nov 3, 4:35 PM
bengotow committed R1:80382c01ce18: Second round of improvements to partition UI (authored by bengotow).
Second round of improvements to partition UI
Tue, Nov 3, 4:35 PM
bengotow requested review of D5005: Second round of improvements to partition UI.
Tue, Nov 3, 12:02 AM

Mon, Nov 2

bengotow accepted D4995: [dagit] Run status tags.

Yep this seems like a great change! My only concern is that the blue for "Started" isn't consistent with the other places we currently use the old status dots (the Run Group sidebar on the Run details page and the Pipeline Overview page are the only two that come to mind), but we can replace those incrementally.

Mon, Nov 2, 7:20 PM
bengotow closed D4898: Change partition matrix click behavior, split large files and GraphQL types.
Mon, Nov 2, 7:18 PM
bengotow committed R1:d62e672d756d: Change partition matrix click behavior, split large files and GraphQL types (authored by bengotow).
Change partition matrix click behavior, split large files and GraphQL types
Mon, Nov 2, 7:17 PM

Fri, Oct 30

bengotow accepted D4981: Add a handful of tests back to App.test.
Fri, Oct 30, 8:22 PM
bengotow updated the diff for D4898: Change partition matrix click behavior, split large files and GraphQL types.

Remove RunGraph class, rename RunGraphUtils > PartitionGraphUtils to reflect the current state of the system, we will reconsider base class / common types for graphs but for now it's more important that the requested types reflect exactly the data we need.

Fri, Oct 30, 8:21 PM
bengotow updated the diff for D4898: Change partition matrix click behavior, split large files and GraphQL types.
  • Fix merge issues
Fri, Oct 30, 7:53 PM
bengotow planned changes to D4898: Change partition matrix click behavior, split large files and GraphQL types.

I don't think I'm qualified to comment on the code, but I love the result!

I would push lightly against including the expectation count / materialization count. Just for the sake of reducing clutter. Though maybe I'm missing some use for them that you had in mind?

Fri, Oct 30, 7:52 PM

Oct 27 2020

bengotow requested review of D4898: Change partition matrix click behavior, split large files and GraphQL types.
Oct 27 2020, 9:22 PM
bengotow accepted D4914: ApolloTestProvider.

This looks good to me - I guess it replaces the MockedProvider we already have in place in App.test.tsx?

Oct 27 2020, 9:04 PM

Oct 26 2020

bengotow accepted D4871: [dagit] Replace Legend tables with Table.

Nice! I think this is a good move 👍 Initially we went with this "cards" appearance for the table rows because the background was super light blue and not white (only the rows were white). Now that all the pages of the app have white backgrounds, the drop shadows and spacing are just adding whitespace.

Oct 26 2020, 3:43 AM
bengotow accepted D4876: [dagit] Quick search component (experiment).

This looks awesome! The components are super clean and I think the SearchResultType and switch statements will make it easy to extend this to new types of results which is great. This is also a very nice use of useReducer in here.

Oct 26 2020, 3:29 AM
bengotow accepted D4885: [dagit] Move fonts to src.

LGTM!

Oct 26 2020, 3:09 AM

Oct 20 2020

bengotow closed D4809: Clean up partitions UI and improve backfill usability.
Oct 20 2020, 10:30 PM
bengotow committed R1:1be29da8b656: Clean up partitions UI and improve backfill usability (authored by bengotow).
Clean up partitions UI and improve backfill usability
Oct 20 2020, 10:29 PM
bengotow updated the diff for D4809: Clean up partitions UI and improve backfill usability.
  • Enable subset selector without previous runs
Oct 20 2020, 7:55 PM
bengotow planned changes to D4809: Clean up partitions UI and improve backfill usability.
Oct 20 2020, 7:40 PM
bengotow added a comment to D4809: Clean up partitions UI and improve backfill usability.

Ahh man, yeah I suppose this could use YAML. I personally prefer the ... syntax because it encodes that the range is inclusive of the end, but that's not hard to see from the live preview. We'll need to reflow the UI a bit because that'll take up a lot more vertical space.

Oct 20 2020, 7:28 PM
bengotow closed D4691: Basic display of asset partition coverage / value by partition.
Oct 20 2020, 7:14 PM
bengotow committed R1:0daf35632658: Basic display of asset partition coverage / value by partition (authored by bengotow).
Basic display of asset partition coverage / value by partition
Oct 20 2020, 7:14 PM
bengotow updated the diff for D4691: Basic display of asset partition coverage / value by partition.

Rebase!

Oct 20 2020, 6:13 PM
bengotow accepted D4828: [dagit] Create instance routes.

This looks good to me!

Oct 20 2020, 6:08 PM
bengotow accepted D4821: [dagit] Create workspace routes.

Just added a couple nits, overall I think this is a huge improvement and we can incrementally improve the UI of each page as we collect feedback.

Oct 20 2020, 6:03 PM

Oct 19 2020

bengotow accepted D4807: (config-scaffold 1/n) Fix config errors visualization in Dagit.

This looks great! Good catch 👍

Oct 19 2020, 9:10 PM
bengotow accepted D4812: remove backfill check for step selection.

Looks great!

Oct 19 2020, 8:38 PM
bengotow updated the diff for D4809: Clean up partitions UI and improve backfill usability.

Simplify state a bit - coerce into a valid state when you make changes so enabled/disabled state of various features don't need to reference many vars

Oct 19 2020, 2:28 PM
bengotow added inline comments to D4809: Clean up partitions UI and improve backfill usability.
Oct 19 2020, 2:08 PM
bengotow planned changes to D4809: Clean up partitions UI and improve backfill usability.

Thanks for the review! Will add the subset w/o previous runs logic changes. Just needed to re-read Sandy's doc now 👍

Oct 19 2020, 1:41 PM
bengotow updated the summary of D4809: Clean up partitions UI and improve backfill usability.
Oct 19 2020, 5:39 AM
bengotow requested review of D4809: Clean up partitions UI and improve backfill usability.
Oct 19 2020, 5:39 AM

Oct 18 2020

bengotow closed D4695: Incrementally fetch data for partition set UI.
Oct 18 2020, 8:29 PM
bengotow committed R1:a384344a470b: Incrementally fetch data for partition set UI (authored by bengotow).
Incrementally fetch data for partition set UI
Oct 18 2020, 8:29 PM
bengotow updated the diff for D4695: Incrementally fetch data for partition set UI.
  • Apply diff feedback
Oct 18 2020, 8:07 PM
bengotow planned changes to D4695: Incrementally fetch data for partition set UI.

Thanks for the feedback! Going to apply everything and then get this merged so I can stack the next round of changes on it.

Oct 18 2020, 7:38 PM

Oct 13 2020

bengotow updated the diff for D4695: Incrementally fetch data for partition set UI.
  • Remove old loader, address diff feedback, add new loading state to chunked query
Oct 13 2020, 8:56 PM
bengotow planned changes to D4695: Incrementally fetch data for partition set UI.

Hey @sashank thanks for the careful review, I appreciate it! Going to address your comments and handle a couple of those edge cases better.

Oct 13 2020, 3:40 PM
bengotow accepted D4709: [dagit] Separate snapshot and current pipeline permalinks.

This looks great! I think the new breadcrumbs approach is a lot more clear. Just left a few inline comments but from a code / usability standpoint I think this is a win.

Oct 13 2020, 3:27 PM
bengotow accepted D4748: [dagit] Warn on tab removal in Playground.

This looks great! Did not realize we had a useConfirmation hook, but that makes this super clean. (Though I assume it's stateless and doesn't only remind you on the removal of the first tab?)

Oct 13 2020, 3:09 PM

Oct 12 2020

bengotow added a reviewer for D4691: Basic display of asset partition coverage / value by partition: sandyryza.
Oct 12 2020, 4:19 PM
bengotow accepted D4747: [dagit] State refactor for execution tabs.

This looks great to me! Just left a few inline comments but nothing requiring any action 👍

Oct 12 2020, 2:46 AM

Oct 7 2020

bengotow accepted D4705: [dagit] Clarify snapshot ID in Runs table.

I think this is a great change—I know Prezi mentioned on the call last thursday that they click the pipeline name by accident too. I like the addition of the Historical tag and I think calling the column "Snapshot" makes it really clear that the underlying system is snapshotting your pipeline each time it's executed, which is cool and under-communicated in our old design. I think it'd be nice to make the click target a bit bigger by merging the run hash and the pipeline name into a single link but that's about it - big wins here!

Oct 7 2020, 9:13 PM
bengotow requested review of D4691: Basic display of asset partition coverage / value by partition.
Oct 7 2020, 12:02 AM

Oct 6 2020

bengotow updated the diff for D4695: Incrementally fetch data for partition set UI.

Fix issue

Oct 6 2020, 11:44 PM
bengotow accepted D4689: [dagit] Fix Assets scrolling.
Oct 6 2020, 11:43 PM
bengotow updated the summary of D4695: Incrementally fetch data for partition set UI.
Oct 6 2020, 11:42 PM
bengotow requested review of D4695: Incrementally fetch data for partition set UI.
Oct 6 2020, 11:41 PM
bengotow closed D4599: Tweak the Gaant layout algorithm to arrange boxes better in Flat mode.
Oct 6 2020, 4:23 PM
bengotow committed R1:ff4132b918f6: Tweak the Gaant layout algorithm to arrange boxes better in Flat mode (authored by bengotow).
Tweak the Gaant layout algorithm to arrange boxes better in Flat mode
Oct 6 2020, 4:23 PM

Oct 5 2020

bengotow accepted D4440: extract run graph utils from partition-specific graphs.

This looks great! I added a few inline comments but I like the idea of creating a base class for these graphs, the chartJS stuff is pretty verbose and it's always a bummer to have a ton of chart config in the middle of a component.

Oct 5 2020, 7:55 PM
bengotow accepted D4667: Don't collect coverage on every Jest run.

LGTM! Thanks for the efforts to make this and buildkite faster, especially for JS-only changes it's been awesome.

Oct 5 2020, 7:32 PM
bengotow accepted D4668: [dagit] Page titles for all routes.

This looks great! In the future it might be a good idea to merge this with RunStatusToPageAttributes, which is responsible for turning the Dagit favicon green / red / gray based on the status of the current run.

Oct 5 2020, 7:30 PM
bengotow accepted D4606: [dagit] Use Blueprint breadcrumbs and tabs for top nav.

This looks really solid! I think the consistency across schedules / assets is a huge win and this ended up being less code than I expected too! 👍

Oct 5 2020, 7:23 PM

Oct 2 2020

bengotow updated the diff for D4599: Tweak the Gaant layout algorithm to arrange boxes better in Flat mode.

Constify FLAT_INSET_FROM_PARENT

Oct 2 2020, 3:26 PM
bengotow added inline comments to D4599: Tweak the Gaant layout algorithm to arrange boxes better in Flat mode.
Oct 2 2020, 3:24 PM
bengotow requested review of D4599: Tweak the Gaant layout algorithm to arrange boxes better in Flat mode.
Oct 2 2020, 4:55 AM
bengotow closed D3882: Make re-execute a two-part button so it’s one-click by default #2687.
Oct 2 2020, 4:46 AM
bengotow committed R1:a63aff6a912a: Make re-execute a two-part button so it’s one-click by default #2687 (authored by bengotow).
Make re-execute a two-part button so it’s one-click by default #2687
Oct 2 2020, 4:46 AM
bengotow updated the diff for D3882: Make re-execute a two-part button so it’s one-click by default #2687.
  • Import ChartJS types
Oct 2 2020, 4:37 AM
bengotow updated the diff for D3882: Make re-execute a two-part button so it’s one-click by default #2687.
  • Import ChartJS types
Oct 2 2020, 4:00 AM
bengotow closed D4462: Add comments to GraphQL definitions of execution params / metadata.
Oct 2 2020, 3:54 AM
bengotow committed R1:67c78163f16f: Add comments to GraphQL definitions of execution params / metadata (authored by bengotow).
Add comments to GraphQL definitions of execution params / metadata
Oct 2 2020, 3:54 AM
bengotow updated the diff for D3882: Make re-execute a two-part button so it’s one-click by default #2687.
  • Pre-rebase changes
  • Merge branch 'master' into dbg/one-click-2
  • Resolve merge conflicts
Oct 2 2020, 3:50 AM
bengotow added a comment to D3882: Make re-execute a two-part button so it’s one-click by default #2687.

Thanks for the feedback folks! I definitely agree Yuhan re: future improvement 👍

Oct 2 2020, 3:49 AM
bengotow updated the diff for D4462: Add comments to GraphQL definitions of execution params / metadata.

Rebase

Oct 2 2020, 2:54 AM

Sep 21 2020

bengotow requested review of D4462: Add comments to GraphQL definitions of execution params / metadata.
Sep 21 2020, 2:18 PM