-
Notifications
You must be signed in to change notification settings - Fork 32
[PB-5542]: add versioning limits context and premium feature gating #1779
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: feature/file-version-history-v3
Are you sure you want to change the base?
[PB-5542]: add versioning limits context and premium feature gating #1779
Conversation
…tory menu with lock state Add VersioningLimitsProvider to DriveView and implement version history menu configuration with locked state support. The version history menu now shows a lock icon and handles locked state when versioning is not available or extension is not allowed.
7ae0637 to
9d6956e
Compare
Deploying drive-web with
|
| Latest commit: |
ea961f7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://088b5735.drive-web.pages.dev |
| Branch Preview URL: | https://feature-file-version-history-fgbg.drive-web.pages.dev |
… context API Replace VersioningLimitsContext with Redux store for consistent state management and improved performance. Add fileVersions slice to cache API responses and prevent redundant fetches. - Create fileVersions Redux slice with caching for versions and limits - Add thunks for fetching file versions and versioning limits - Implement cache invalidation on version create, delete, and restore - Optimize Sidebar to use cached data and avoid infinite loops - Memoize VersionItem component to prevent unnecessary re-renders - Remove VersioningLimitsContext and consolidate all state in Redux - Update all components to use Redux selectors instead of context
Tests will be added in a separate PR. |
| () => moveDestinationFolderId ?? currentFolderId, | ||
| [moveDestinationFolderId, currentFolderId], | ||
| ); | ||
| const limits = useAppSelector((state: RootState) => state.fileVersions.limits); |
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.
create selector for get fields of a redux state, I see that this state is accessed from diferent files
|
|
||
| interface FileVersionsState { | ||
| versionsByFileId: Record<string, FileVersion[]>; | ||
| loadingStates: Record<string, 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.
This could lead to confusion. loadingState of what? maybe something as isLoadingFileById left clear what does, it is more verbose but more clear the purpose, and add to Record type the FileVersion['id'] field instead of string to make it more clearer.
Could do the same with versionsByFileId
| versionsByFileId: Record<string, FileVersion[]>; | ||
| loadingStates: Record<string, boolean>; | ||
| errors: Record<string, string | null>; | ||
| limits: GetFileLimitsResponse | null; |
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.
GetFileLimitsResponse seems a function name, not a type nam.
The type should be something like FileLimitsResponse
| interface FileVersionsState { | ||
| versionsByFileId: Record<string, FileVersion[]>; | ||
| loadingStates: Record<string, boolean>; | ||
| errors: Record<string, string | null>; |
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.
same here with string and file id, and the name of the variable
| loadingStates: Record<string, boolean>; | ||
| errors: Record<string, string | null>; | ||
| limits: GetFileLimitsResponse | null; | ||
| limitsLoading: 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.
for boleean use the standard convention, isLimitsLoading
src/services/date.service.ts
Outdated
| return dayjs(date).format(`D MMM, YYYY [${translatedAt}] HH:mm`); | ||
| }; | ||
|
|
||
| function getDaysUntilExpiration(expiresAt: Date | string): number { |
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.
use arrow function format
| locked?: boolean; | ||
| onLockedClick?: () => void; | ||
| allowedExtension?: 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.
Remember that Booleans should be read as a question (using prefixes such as is, has, can)
| const getVersionHistoryMenuItem = ( | ||
| viewVersionHistory: (target?) => void, | ||
| config?: VersionHistoryMenuConfig, | ||
| ): MenuItemType<DriveItemData> => { | ||
| const isLocked = config?.locked ?? false; | ||
| const allowedExtension = config?.allowedExtension ?? true; | ||
| const action = isLocked ? (config?.onLockedClick ?? (() => undefined)) : viewVersionHistory; | ||
| const IconComponent = isLocked ? LockSimple : ClockCounterClockwise; | ||
|
|
||
| const isVersionHistoryUnavailable = (item: DriveItemData) => { | ||
| const isFolder = item.isFolder; | ||
| const isUnsupportedExtension = !allowedExtension; | ||
| const isDisabledForUnlockedItem = !isLocked && (isFolder || isUnsupportedExtension); | ||
| return isDisabledForUnlockedItem; | ||
| }; | ||
|
|
||
| return { | ||
| name: t('drive.dropdown.versionHistory'), | ||
| icon: IconComponent, | ||
| action, | ||
| disabled: isVersionHistoryUnavailable, | ||
| ...(isLocked && { | ||
| locked: true, | ||
| node: ( | ||
| <div | ||
| className="flex flex-row items-center space-x-2" | ||
| data-locked="true" | ||
| style={{ opacity: 0.5, cursor: 'default' }} | ||
| > | ||
| <IconComponent size={20} /> | ||
| <span>{t('drive.dropdown.versionHistory')}</span> | ||
| </div> | ||
| ), | ||
| }), | ||
| } as MenuItemType<DriveItemData> & { locked?: 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.
this function seems a bit chaotic, I'll propose to change to something similar to this:
export type VersionHistoryMenuConfig = {
isLocked: boolean;
isExtensionAllowed: boolean;
onUpgradeClick?: () => void;
};
const getVersionHistoryMenuItem = (
viewVersionHistory: (target?) => void,
config?: VersionHistoryMenuConfig,
): MenuItemType<DriveItemData> => {
const isLocked = config?.isLocked ?? false;
const isExtensionAllowed = config?.isExtensionAllowed ?? true;
const onUpgradeClick = config?.onUpgradeClick;
if (isLocked) {
return {
name: t('drive.dropdown.versionHistory'),
icon: LockSimple,
action: onUpgradeClick,
disabled: () => false,
className: 'opacity-50 cursor-pointer',
};
}
return {
name: t('drive.dropdown.versionHistory'),
icon: ClockCounterClockwise,
action: viewVersionHistory,
disabled: (item: DriveItemData) => item.isFolder || !isExtensionAllowed,
};
};
Perhaps I'm missing something, let me know
- Create fileVersions selectors for centralized state access
- Rename state properties for clarity:
- loadingStates → isLoadingByFileId
- errors → errorsByFileId
- limitsLoading → isLimitsLoading
- Update type imports to use FileLimitsResponse (SDK v1.11.24)
- Use NonNullable for Record keys to prevent null index types
- Refactor version history menu config:
- Rename properties: locked → isLocked, allowedExtension → isExtensionAllowed, onLockedClick → onUpgradeClick
- Simplify getVersionHistoryMenuItem with early return pattern
- Replace direct state access with selectors across components
- Export getDaysUntilExpiration utility function
- Add type assertion for restored version fileId
| const versions = | ||
| useAppSelector((state: RootState) => (item ? fileVersionsSelectors.getVersionsByFileId(state, item.uuid) : [])) || | ||
| []; | ||
| const isLoading = | ||
| useAppSelector((state: RootState) => (item ? fileVersionsSelectors.isLoadingByFileId(state, item.uuid) : false)) || | ||
| false; |
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.
If the undefined return is not necessaryin getVersionsByFileId selector, the alternative return of [] could be added to the selectors when it does not exist, as has been done in isLoadingByFileId.
That way, the logical OR operator here will not be necessary and could be removed, which will make the code cleaner
| node: ( | ||
| <div | ||
| className="flex flex-row items-center space-x-2" | ||
| data-locked="true" | ||
| style={{ opacity: 0.5, cursor: 'default' }} | ||
| > | ||
| <LockSimple size={20} /> | ||
| <span>{t('drive.dropdown.versionHistory')}</span> | ||
| </div> | ||
| ), | ||
| }; |
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.
why is this component necesary? with only passing icon field is not displayed correctly?
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.
It’s necessary for the option to look disabled but still be clickable to open the plans modal
|


Description
Related Issues
Related Pull Requests
Checklist
Testing Process
Additional Notes