-
Notifications
You must be signed in to change notification settings - Fork 3
[UXIT-3480] Refactor upload progress logic and add “Publishing” badge state #76
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?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Filecoin Pin UploadIPFS Artifacts:
Onchain verification:
Payment:
|
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.
I am a big fan of these changes.. I love the cleanup here and consolidation of the data model.
left a few comments
src/components/layout/content.tsx
Outdated
| onToggleExpanded={() => setActiveUploadExpanded(!activeUploadExpanded)} | ||
| pieceCid={activeUpload.pieceCid ?? ''} | ||
| progresses={activeUpload.progress} | ||
| stepStates={activeUpload.progress} |
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.
maybe align .progress name here to state as well?
| stepStates={activeUpload.progress} | |
| stepStates={activeUpload.state} |
| ) | ||
| const uploadingStep = progresses.find((p) => p.step === 'uploading-car') | ||
| const announcingStep = progresses.find((p) => p.step === 'announcing-cids') | ||
| const { firstStageProgress, firstStageStatus, hasUploadIpniFailure } = useUploadProgress({ |
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.
might be an opportunity here to align StageProgress and StageStatus ? seems redundant at this point.
| getCombinedFirstStageProgress={getCombinedFirstStageProgress} | ||
| getCombinedFirstStageStatus={getCombinedFirstStageStatus} | ||
| progresses={combinedProgresses} | ||
| firstStageProgress={firstStageProgress} | ||
| firstStageStatus={firstStageStatus} | ||
| stepStates={firstStageStates} |
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.
do we need StageProgress, stageStatus, and stepStates now? if we have a pure function/hook that can derive firstStageProgress and firstStageStatus from stepStates then we can remove two more drilled props..
maybe not necessary now (potential future improvement.. if it helps readability/maintainence/testability), just thinking out loud.
| firstStageStatus: StepState['status'] | ||
| firstStageProgress: StepState['progress'] |
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.
I know we already had this code (and I think I added it), but I have not been a fan of explicitly declaring the "firstStage" as it's own prop.. ideally this would be codified in the "StepState" data model and handled appropriately, rather than this explicit naming of the first step.
ultimately though, the goal is simplicity since we want folks to be able to follow the code here easily.
src/hooks/use-upload-progress.ts
Outdated
| firstStageProgress: StepState['progress'] | ||
| firstStageStatus: StepState['status'] | ||
| hasUploadIpniFailure: boolean | ||
| isUploadSuccessful: boolean | ||
| isUploadFailure: boolean | ||
| uploadBadgeStatus: Status |
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.
we should add jsdocs to explain what each of these mean
src/hooks/use-upload-progress.ts
Outdated
| hasError: boolean | ||
| firstStageProgress: StepState['progress'] | ||
| firstStageStatus: StepState['status'] | ||
| hasUploadIpniFailure: boolean |
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.
| hasUploadIpniFailure: boolean | |
| hasIpniAnnounceFailure: boolean |
is a little more descriptive. we don't actually "upload" to IPNI
src/hooks/use-upload-progress.ts
Outdated
| const firstStageProgress = getFirstStageProgress(stepStates) | ||
| const firstStageStatus = getFirstStageStatus(stepStates) | ||
| const { hasUploadIpniFailure, isUploadSuccessful, isUploadFailure } = getUploadOutcome({ stepStates, cid }) | ||
| const uploadBadgeStatus = getUploadBadgeStatus({ | ||
| isUploadSuccessful, | ||
| isUploadFailure, | ||
| stepStates, | ||
| }) |
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.
I love the cleaner code inside this hook with isolated functions for getting each part defined elsewhere. This will be much easier to test.
| export const STAGE_STEPS: Record<StageId, readonly StepState['step'][]> = { | ||
| 'first-stage': ['creating-car', 'checking-readiness', 'uploading-car'], | ||
| 'second-stage': ['announcing-cids'], | ||
| 'third-stage': ['finalizing-transaction'], | ||
| } as const | ||
|
|
||
| export type FirstStageGroup = Record< | ||
| 'creatingCar' | 'checkingReadiness' | 'uploadingCar', |
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.
we could probably use this opportunity to just use camelCase for the steps instead of kebab-case.
also, firstStageGroup isn't tied to STAGE_STEPS currently and we should lock it in to prevent breakage in the future.
check out typescript playground
src/utils/upload/upload-outcome.ts
Outdated
| const finalizingTransactionStep = stepStates.find((stepState) => stepState.step === 'finalizing-transaction') | ||
| const announcingCidsStep = stepStates.find((stepState) => stepState.step === 'announcing-cids') | ||
|
|
||
| if (isUploadSuccessful) return 'pinned' |
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 seems like it could still show 'pinned' even if announcingCidsStep is incomplete. we should check for that first right?
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.
Super cool Barbara, great one 🎉
| 'in-progress': 'In progress', | ||
| completed: 'Complete', | ||
| pinned: 'Pinned', | ||
| published: 'Published', |
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.
No Publishing state?
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.
Sorry, can you clarify for me?
|
|
||
| export interface StepState { | ||
| step: StepName | ||
| progress: number // 0–100 |
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.
How about we properly type progress?
type Progress =
| 0
| 1
| 2
| 3
| 4
| 5
| 6
| 7
| 8
| 9
| 10
| 11
| 12
| 13
| 14
| 15
| 16
| 17
| 18
| 19
| 20
| 21
| 22
| 23
| 24
| 25
| 26
| 27
| 28
| 29
| 30
| 31
| 32
| 33
| 34
| 35
| 36
| 37
| 38
| 39
| 40
| 41
| 42
| 43
| 44
| 45
| 46
| 47
| 48
| 49
| 50
| 51
| 52
| 53
| 54
| 55
| 56
| 57
| 58
| 59
| 60
| 61
| 62
| 63
| 64
| 65
| 66
| 67
| 68
| 69
| 70
| 71
| 72
| 73
| 74
| 75
| 76
| 77
| 78
| 79
| 80
| 81
| 82
| 83
| 84
| 85
| 86
| 87
| 88
| 89
| 90
| 91
| 92
| 93
| 94
| 95
| 96
| 97
| 98
| 99
| 100Or perhaps it doesn't to be as granular:
type Progress = 0 | 10 | 20 | 30 | 40 | 50 | 60 | 70 | 80 | 90 | 100There 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.
there are so many cool utility types you can create with some infer and other typescript magic.. https://stackoverflow.com/questions/39494689/is-it-possible-to-restrict-number-to-a-certain-range
but this feels like overkill?
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.
Maybe it is yes.
…ndling This update transitions the upload progress management from using the Progress type to the new StepState type, enhancing type safety and clarity. The changes include updates to various components and hooks to accommodate the new structure, ensuring consistent handling of upload steps and statuses throughout the application.
…anagement This update introduces the use of custom hooks for managing step states and first stage progress, improving the clarity and maintainability of the upload process. The changes include refactoring components to utilize the new hooks, ensuring consistent handling of upload steps and statuses across the application. Additionally, the naming conventions for stage identifiers have been updated for better readability.
…cture This update refactors the upload-related utility functions by consolidating them into more descriptive files, enhancing clarity and maintainability. The changes include updating import paths in various components and hooks to reflect the new structure, ensuring consistent usage of the updated utility functions across the application.
39ad9ab to
83ea1dc
Compare
This update refines the UploadOutcome type by exporting it for broader use and enhances the getUploadBadgeStatus function to utilize the new type properties. The changes improve type safety and clarity in the upload progress handling, ensuring consistent status management across the application.
|
Thank you both for the comments! I've made additional improvements:
@SgtPooki if you agree, I'll tackle "Consider using camelCase for steps instead of kebab-case" in a separate PR 🙏 |
Description
use-upload-progresshook andcomponents)Key Changes
Step → the smallest unit of work (
creating-car,announcing-cids, etc.)Stage → a grouping of one or more steps
first-stage=creating-car,checking-readiness,uploading-carsecond-stage=announcing-cidsthird-stage=finalizing-transactionUpload → the complete process made up of all stages
Defined explicit
StepState,StageState, andUploadStatetypes to reflect this structure.src/types/upload/andsrc/utils/upload/directory:progresses→stepStatesto reflect that these hold full step state objects, not just progress values.calculateCompletionStates→getUploadOutcomefor clearer intent.getStageBadgeStatus→getUploadBadgeStatussince the badge reflects overall upload state, not an individual stage.publishingprogress more accurately: