diff --git a/rfcs/086-item-viewer-refactor/06-phase-0-type-audit.md b/rfcs/086-item-viewer-refactor/06-phase-0-type-audit.md index 6769c69f..b4da52b2 100644 --- a/rfcs/086-item-viewer-refactor/06-phase-0-type-audit.md +++ b/rfcs/086-item-viewer-refactor/06-phase-0-type-audit.md @@ -209,12 +209,12 @@ Your Catalogue API types (`Work`, `WorkBasic`, `Item`, etc.) are manually define ## Acceptance Criteria -- [ ] Fixed both `setSearchResults` implicit `any` types -- [ ] (Optional) Added `ImageService` type to `content/webapp/types/item-viewer.ts` -- [ ] Documented existing type structure for reference -- [ ] Checked for duplicate type definitions across files -- [ ] `yarn tsc` runs without errors in ItemViewer files -- [ ] All team members understand what types exist and where they live +- [x] Fixed both `setSearchResults` implicit `any` types - now `(v: SearchResults | null) => void` in both context files +- [x] (Optional) Added `ImageService` type to `content/webapp/types/item-viewer.ts` - used as `PartialImageService = Pick` +- [x] Documented existing type structure for reference +- [x] Checked for duplicate type definitions across files +- [x] `yarn tsc` runs without errors in ItemViewer files +- [ ] All team members understand what types exist and where they live - not something the code can evidence, left for the team to judge ## What This Phase Achieves diff --git a/rfcs/086-item-viewer-refactor/07-phase-1-feature-flag.md b/rfcs/086-item-viewer-refactor/07-phase-1-feature-flag.md index 5b90f8cf..d6135851 100644 --- a/rfcs/086-item-viewer-refactor/07-phase-1-feature-flag.md +++ b/rfcs/086-item-viewer-refactor/07-phase-1-feature-flag.md @@ -125,15 +125,17 @@ import ItemViewerContextV2 from '@weco/common/contexts/ItemViewerContextV2'; ## Success Criteria -- [ ] Feature flag defined in `toggles.ts` -- [ ] ItemViewerContextV2 created (identical to ItemViewerContext) -- [ ] All original components renamed to `.legacy.tsx` -- [ ] Wrapper component loads legacy or refactored based on flag -- [ ] All `.refactored.tsx` files created and import ItemViewerContextV2 -- [ ] Application runs with flag OFF (uses legacy) -- [ ] Application runs with flag ON (uses refactored, identical behaviour) -- [ ] No TypeScript errors -- [ ] No console warnings +Met, though several were delivered under different names than planned - see the naming note in the [README](./README.md). + +- [x] Feature flag defined in `toggles.ts` - as `itemViewerRefactor` +- [x] ItemViewerContextV2 created (identical to ItemViewerContext) - built as `contexts/ItemViewerContext/refactored.tsx`, with `legacy.tsx` beside it +- [x] All original components renamed to `.legacy.tsx` - moved into a `legacy/` directory instead of renamed +- [x] Wrapper component loads legacy or refactored based on flag - `IIIFViewer/index.tsx` +- [x] All `.refactored.tsx` files created and import ItemViewerContextV2 - created under `refactored/` +- [x] Application runs with flag OFF (uses legacy) +- [x] Application runs with flag ON (uses refactored, identical behaviour) +- [x] No TypeScript errors +- [ ] No console warnings - a temporary log reporting which context is in use was added deliberately, in the context barrel. Removing it is item 1 of [#13273](https://github.com/wellcomecollection/wellcomecollection.org/issues/13273), and has to happen before the toggle goes on publicly. ## Next Steps diff --git a/rfcs/086-item-viewer-refactor/08-phase-2-split-components.md b/rfcs/086-item-viewer-refactor/08-phase-2-split-components.md index c216dd8d..10a3051f 100644 --- a/rfcs/086-item-viewer-refactor/08-phase-2-split-components.md +++ b/rfcs/086-item-viewer-refactor/08-phase-2-split-components.md @@ -220,12 +220,12 @@ After this phase, consider splitting other components with multiple modes: ## Success Criteria -- [ ] `MainViewer` is a simple router (<20 lines) -- [ ] `VirtualizedImageViewer` handles only image-only works -- [ ] `PaginatedItemViewer` handles only mixed/archive works -- [ ] All tests pass -- [ ] No behavioral changes detected -- [ ] Code is easier to understand and test +- [x] `MainViewer` is a simple router (<20 lines) - it routes and does nothing else, but the file is 32 lines: about 14 of routing, the rest imports and a styled container +- [x] `VirtualizedImageViewer` handles only image-only works +- [x] `PaginatedItemViewer` handles only mixed/archive works +- [x] All tests pass +- [x] No behavioral changes detected +- [x] Code is easier to understand and test ## Time Estimate diff --git a/rfcs/086-item-viewer-refactor/09-phase-3-canvas-data.md b/rfcs/086-item-viewer-refactor/09-phase-3-canvas-data.md index 8e3b9996..e6e14806 100644 --- a/rfcs/086-item-viewer-refactor/09-phase-3-canvas-data.md +++ b/rfcs/086-item-viewer-refactor/09-phase-3-canvas-data.md @@ -132,7 +132,7 @@ All tests should pass. ### 1.6 (Optional) Manual Testing -See [13-testing-strategy.md](./13-testing-strategy.md) for comprehensive manual testing checklist. +See [14-testing-strategy.md](./14-testing-strategy.md) for comprehensive manual testing checklist. **Quick smoke tests:** - [ ] Viewer loads correctly @@ -158,10 +158,10 @@ Self-documenting code! No mental gymnastics required. ## Success Criteria -- [ ] All automated tests pass (context + components) -- [ ] TypeScript compiles with no errors -- [ ] Components use context values (not calculating locally) -- [ ] Feature flag toggle works (old vs new identical behaviour) +- [x] All automated tests pass (context + components) +- [x] TypeScript compiles with no errors +- [x] Components use context values (not calculating locally) - with two deliberate exceptions under principle 7: `canNavigateNext`/`canNavigatePrevious` stay local to `ViewerBottomBar`, their only consumer, derived from the shared `totalCanvases`; and `hasIiifImageService` stays local to `IIIFViewer`. `currentCanvasIndex` was never added at all - see the [README](./README.md). +- [x] Feature flag toggle works (old vs new identical behaviour) - [ ] Manual testing checklist complete (optional) ## Time Breakdown diff --git a/rfcs/086-item-viewer-refactor/10-phase-4-download-logic.md b/rfcs/086-item-viewer-refactor/10-phase-4-download-logic.md index a09dbc48..f46c5a70 100644 --- a/rfcs/086-item-viewer-refactor/10-phase-4-download-logic.md +++ b/rfcs/086-item-viewer-refactor/10-phase-4-download-logic.md @@ -154,10 +154,10 @@ All should pass. ## Success Criteria -- [ ] Tests for `useDownloadOptions` pass -- [ ] `ViewerTopBar` ~65 lines shorter -- [ ] Download dropdown still works identically -- [ ] All download types still appear correctly +- [x] Tests for `useDownloadOptions` pass +- [x] `ViewerTopBar` ~65 lines shorter +- [x] Download dropdown still works identically +- [x] All download types still appear correctly ## Time Breakdown diff --git a/rfcs/086-item-viewer-refactor/11-phase-5-restriction-status.md b/rfcs/086-item-viewer-refactor/11-phase-5-restriction-status.md index 34d669da..6b9bd415 100644 --- a/rfcs/086-item-viewer-refactor/11-phase-5-restriction-status.md +++ b/rfcs/086-item-viewer-refactor/11-phase-5-restriction-status.md @@ -69,9 +69,9 @@ const { isCurrentCanvasRestricted } = useItemViewerContextV2(); ## Success Criteria -- [ ] `isCurrentCanvasRestricted` in context -- [ ] `ViewerTopBar` uses context value -- [ ] Restricted badge appears correctly +- [x] `isCurrentCanvasRestricted` in context +- [x] `ViewerTopBar` uses context value +- [ ] Restricted badge appears correctly - there is no restricted badge, in either tree. What `isCurrentCanvasRestricted` actually gates in `ViewerTopBar` is hiding the download button on a restricted canvas, unless the user is staff with restricted access. ## Time: ~1 hour diff --git a/rfcs/086-item-viewer-refactor/12-phase-6-duplicate-calls.md b/rfcs/086-item-viewer-refactor/12-phase-6-duplicate-calls.md index d7bb7967..19fdec85 100644 --- a/rfcs/086-item-viewer-refactor/12-phase-6-duplicate-calls.md +++ b/rfcs/086-item-viewer-refactor/12-phase-6-duplicate-calls.md @@ -5,77 +5,42 @@ **Effort:** 30 minutes **Risk:** Low **Priority:** Medium +**Status:** Done - [#12989](https://github.com/wellcomecollection/wellcomecollection.org/issues/12989) **Previous:** [Phase 5: Restriction Status](./11-phase-5-restriction-status.md) **Next:** [Phase 7: Cleanup](./13-phase-7-cleanup.md) ## Goal -Since `currentCanvasIndex` is now in context (from Phase 1), remove all duplicate `queryParamToArrayIndex(query.canvas)` calls across components. +Remove duplicated calculations of "which canvas is currently showing" across components. -## Why? +## What this phase originally assumed -**Before:** -```typescript -// Thumbnails.tsx -const index = queryParamToArrayIndex(query.canvas); +As planned, this was a find-and-replace: since `currentCanvasIndex` was to be on the context, swap every `queryParamToArrayIndex(query.canvas)` call for it, in `Thumbnails.tsx`, `NoScriptImage.tsx` and `MultipleManifestList.tsx`. -// NoScriptImage.tsx -const index = queryParamToArrayIndex(query.canvas); +Neither half of that held by the time the phase came round: -// MultipleManifestList.tsx -const index = queryParamToArrayIndex(query.canvas); -``` +- `currentCanvasIndex` was deliberately never added to the context. It would duplicate `query.canvas`, which is already there, and the 1-based canvas number and the 0-based array index are kept as distinct concepts throughout. See the note in [README](./README.md). +- None of the three named files calculate a canvas index any more. `Thumbnails.tsx` calls `queryParamToArrayIndex` on `query.page` and `MultipleManifestList.tsx` on `query.manifest` - different query params, unrelated to the current canvas - and `NoScriptImage.tsx` doesn't call it at all, having moved to the shared `getCanvasesForPage` helper in Phase 3. -**Problem:** Same calculation repeated in 3+ files. +So the phase became an audit of the whole `refactored/` directory instead. -**After:** -```typescript -const { currentCanvasIndex } = useItemViewerContextV2(); -``` +## What the audit found -Single source of truth! +One genuine duplicate, in `GridViewer.tsx`: the memoised `Cell` recalculated `queryParamToArrayIndex(query.canvas)` for its own `aria-current`, once per rendered grid cell, while the parent already calculated the identical value for its scroll-to-row effect. Fixed by calculating it once in `GridViewer` and passing it down through `itemData`, plus a first test for that component. -## Files to Update +Four lookalikes were checked and deliberately left alone, being different logic that happens to read similarly: -Find all occurrences: - -```bash -cd content/webapp -grep -r "queryParamToArrayIndex(query.canvas)" . -``` - -### Expected files (update each): - -1. **`Thumbnails.tsx`** (lines 38, 49) -2. **`NoScriptImage.tsx`** (line 42) -3. **`MultipleManifestList.tsx`** (lines 23, 37) -4. Any others found - -### Replace pattern: - -```typescript -// OLD: -const index = queryParamToArrayIndex(query.canvas); -// or -queryParamToArrayIndex(query.canvas) - -// NEW: -const { currentCanvasIndex } = useItemViewerContextV2(); -// or just use currentCanvasIndex directly -``` - -## Testing - -- [ ] Thumbnails highlight correct canvas -- [ ] NoScript image shows correct canvas -- [ ] Multiple manifest list shows correct position -- [ ] All components that use index still work +- `GridViewer`'s `Cell` and `VirtualizedImageViewer`'s `ItemRenderer` each have a local `currentCanvas`, but those are per-cell and per-row canvases from iterating the whole list, not the one current canvas on the context. +- `VirtualizedImageViewer` calls `hasRestrictedItem(canvases[0])`, which is about the first canvas specifically, for a margin adjustment - not the current one. +- `IIIFViewer.tsx` reads `document.fullscreenElement` directly, to skip resize handling while a native video player is fullscreen. That's a different question from the fullscreen toggle state centralised in Phase 3. ## Success Criteria -- [ ] No more `queryParamToArrayIndex(query.canvas)` calls in consuming components -- [ ] All components use `currentCanvasIndex` from context -- [ ] Test coverage maintained +The criteria as originally written, with what actually happened against each: + +- [x] No more `queryParamToArrayIndex(query.canvas)` calls in consuming components - met, though by this point the only one left was the `GridViewer.tsx` duplicate above +- [ ] All components use `currentCanvasIndex` from context - not done, and no longer wanted: the value was deliberately never added to the context +- [x] Test coverage maintained - `GridViewer.test.tsx` added, the component's first test ## Time: ~30 minutes diff --git a/rfcs/086-item-viewer-refactor/13-migration-checklist.md b/rfcs/086-item-viewer-refactor/13-migration-checklist.md index 1ded3b80..22864774 100644 --- a/rfcs/086-item-viewer-refactor/13-migration-checklist.md +++ b/rfcs/086-item-viewer-refactor/13-migration-checklist.md @@ -4,6 +4,8 @@ This checklist helps you track progress through each phase. Check off items as you complete them. +Phases 0-6 are done; only Phase 7 is outstanding. The fine-grained boxes below are left as originally written rather than ticked retrospectively - each phase document's own success criteria have been checked off against the code instead, including the handful that weren't met as specified. Phase 6 and Phase 7 here have been rewritten, because their items no longer described the work to be done. + ## Before Starting - [ ] Create feature branch: `refactor/iiif-viewer-context` @@ -163,13 +165,13 @@ Duration: 1-1.5 hours ## Phase 6: Duplicate Index Calls -- [ ] Find all `queryParamToArrayIndex(query.canvas)` calls -- [ ] Update Thumbnails.tsx to use `currentCanvasIndex` from context -- [ ] Update NoScriptImage.tsx to use `currentCanvasIndex` -- [ ] Update MultipleManifestList.tsx to use `currentCanvasIndex` -- [ ] Update any other files found -- [ ] Test thumbnails highlight correctly -- [ ] Test all navigation works +This ran as an audit rather than the find-and-replace planned here: `currentCanvasIndex` was never added to the context, and none of the three files named below calculate a canvas index any more. See [Phase 6](./12-phase-6-duplicate-calls.md) for what the audit found. + +- [x] Find all `queryParamToArrayIndex(query.canvas)` calls +- [x] Audit the whole `refactored/` directory for duplicated current-canvas calculations +- [x] Deduplicate the one real case, in `GridViewer.tsx` +- [x] Test thumbnails highlight correctly +- [x] Test all navigation works **Time checkpoint:** Should take ~30 minutes @@ -179,14 +181,14 @@ Duration: 1-1.5 hours **Only do this after toggle defaults to ON for 1+ week with no issues!** -- [ ] Remove feature flag from `toggles.ts` -- [ ] Delete all `.legacy.tsx` files- [ ] Rename `.refactored.tsx` to `.tsx` -- [ ] Delete wrapper `index.tsx` -- [ ] Delete old `ItemViewerContext` directory -- [ ] Rename `ItemViewerContextV2` to `ItemViewerContext` -- [ ] Update all imports from V2 to standard -- [ ] Rename test files (remove .refactored) -- [ ] Update test imports +- [ ] Remove the `itemViewerRefactor` flag from `toggles/webapp/toggles.ts`, and redeploy the toggles package +- [ ] Delete `IIIFViewer/legacy/` and `contexts/ItemViewerContext/legacy.tsx` +- [ ] Delete the `IIIFViewer/index.tsx` switch and move `refactored/`'s contents up a level +- [ ] Fold `contexts/ItemViewerContext/refactored.tsx` into `index.tsx`, dropping the flag-switching barrel +- [ ] Remove `isRefactoredContext` and the migration console log +- [ ] Put every consumer on one context import path +- [ ] Collapse the dual-context test harness in `test/fixtures/iiif/render.tsx` +- [ ] Drop the per-test `jest.mock` of `useFeatureFlags` and the `useRefactoredContext` arguments - [ ] Run all tests - still pass - [ ] `yarn tsc` - no errors - [ ] Application runs correctly diff --git a/rfcs/086-item-viewer-refactor/13-phase-7-cleanup.md b/rfcs/086-item-viewer-refactor/13-phase-7-cleanup.md index 81956259..c6da25b3 100644 --- a/rfcs/086-item-viewer-refactor/13-phase-7-cleanup.md +++ b/rfcs/086-item-viewer-refactor/13-phase-7-cleanup.md @@ -5,11 +5,12 @@ **Effort:** 1-2 hours **Risk:** Low **Priority:** Required (after toggle has been defaulted to ON) +**Status:** Not started - [#12990](https://github.com/wellcomecollection/wellcomecollection.org/issues/12990) **Previous:** [Phase 6: Duplicate Index Calls](./12-phase-6-duplicate-calls.md) ## Goal -Clean up feature flag infrastructure after successful adoption. Remove legacy code, rename refactored files, and finalize the implementation. +Clean up feature flag infrastructure after successful adoption. Remove legacy code, promote the refactored tree to be the only one, and finalise the implementation. ## When to Do This @@ -19,113 +20,99 @@ Clean up feature flag infrastructure after successful adoption. Remove legacy co - All metrics stable - Team confidence is high +Note that the toggle can't go on publicly until the temporary badges and console logs are removed - see item 1 of [#13273](https://github.com/wellcomecollection/wellcomecollection.org/issues/13273). + ## Steps -### 5.1 Remove Feature Flag +The steps below were rewritten once Phases 0-6 were done, because the original ones referenced a file layout that was never built (`ItemViewerContextV2`, `.legacy.tsx`/`.refactored.tsx` filename suffixes). The paths here are the real ones. + +### 7.1 Remove the feature flag -**File:** `toggles/webapp/app/toggles.ts` +**File:** `toggles/webapp/toggles.ts` ```typescript -// DELETE: -iiifViewerRefactored: { - id: 'iiifViewerRefactored', - title: 'IIIF Viewer - Refactored Context', - defaultValue: false, - description: 'Use refactored ItemViewerContext with centralised derived values', +// DELETE from featureFlags: +{ + id: 'itemViewerRefactor', + title: 'Item viewer refactor', + initialValue: false, + description: + 'Displays the refactored item viewer instead of the current one.', + type: 'experimental', }, ``` -### 5.2 Delete Legacy Files +Deploy the toggles package afterwards, so the flag stops being served from `toggles.wellcomecollection.org/toggles.json`. -```bash -cd content/webapp/views/pages/works/work/IIIFViewer +### 7.2 Delete the legacy viewer and its context -# Delete all .legacy.tsx files -rm IIIFViewer.legacy.tsx -rm ViewerTopBar.legacy.tsx -rm ZoomedImage.legacy.tsx -rm MainViewer.legacy.tsx -# ... any other .legacy.tsx files +```bash +cd content/webapp +rm -r views/pages/works/work/IIIFViewer/legacy +rm contexts/ItemViewerContext/legacy.tsx ``` -Also remove types that are not in use anymore. +Then delete any types that only legacy used. -### 5.3 Rename Refactored Files to Standard Names +### 7.3 Promote the refactored viewer -```bash -# Rename .refactored.tsx to .tsx -mv IIIFViewer.refactored.tsx IIIFViewer.tsx -mv ViewerTopBar.refactored.tsx ViewerTopBar.tsx -mv ZoomedImage.refactored.tsx ZoomedImage.tsx -mv MainViewer.refactored.tsx MainViewer.tsx -``` - -### 5.4 Delete Wrapper Component +`views/pages/works/work/IIIFViewer/index.tsx` is the `dynamic()` switch between the two trees. It goes, and `refactored/index.tsx` takes its place: ```bash -# No longer needed - IIIFViewer.tsx is now the main component -rm index.tsx # The wrapper that switched between legacy/refactored +cd content/webapp/views/pages/works/work/IIIFViewer +rm index.tsx +mv refactored/* . +rmdir refactored ``` -### 5.5 Rename Context +### 7.4 Collapse the context -```bash -# Delete old context -rm -rf content/webapp/contexts/ItemViewerContext +`contexts/ItemViewerContext/index.tsx` is a barrel that picks legacy or refactored on the flag. With legacy gone it has no job: fold `refactored.tsx` into `index.tsx` and delete the barrel's switching logic. -# Rename new context to standard name -mv content/webapp/contexts/ItemViewerContextV2 \ - content/webapp/contexts/ItemViewerContext -``` +While doing so, remove: -### 5.6 Update All Imports +- the `console.log` that reports which context is in use, and the `window.__ivr_context_logged` global declaration that guards it (both marked TODO in the file) +- `isRefactoredContext` from the context type and its default value - the discriminant only existed to narrow the legacy-or-refactored union -Replace all occurrences of `ItemViewerContextV2` with `ItemViewerContext`: +### 7.5 Consolidate the import paths -```bash -# Find all imports -grep -r "ItemViewerContextV2" content/webapp +Components currently reach the context two ways, and both need to end up on one path: -# Update each file: -# OLD: import ItemViewerContextV2 from '@weco/common/contexts/ItemViewerContextV2'; -# NEW: import ItemViewerContext from '@weco/common/contexts/ItemViewerContext'; +```bash +cd content/webapp +grep -rl "contexts/ItemViewerContext'" --include="*.ts" --include="*.tsx" . # via the barrel +grep -rl "contexts/ItemViewerContext/refactored" --include="*.ts" --include="*.tsx" . ``` -### 5.7 Remove Unused Props +At the time of writing that's 32 files on the barrel and 16 importing `refactored` directly. -Identify components that can drop props now that context provides the data: +### 7.6 Simplify the test harness -```typescript -// Example: ViewerTopBar might have received props that context now provides -// Remove those props from the interface and component -``` +`test/fixtures/iiif/render.tsx` carries the migration's dual-context machinery: `renderWithContext`'s `useRefactoredContext` option, the `RenderWithContextOptions` discriminated union, `RenderWithRefactoredContextOptions`, and separate legacy/refactored mock context factories. All of that collapses to a single context. -### 5.8 Update Tests +Individual test files then drop the `jest.mock` of `useFeatureFlags` that forces `itemViewerRefactor: true`, and the `useRefactoredContext: true` argument at each call site. -Rename test files and update imports: +### 7.7 Remove unused props -```bash -mv ItemViewerContextV2.test.tsx ItemViewerContext.test.tsx -mv IIIFViewer.refactored.test.tsx IIIFViewer.test.tsx -# Update imports in test files from V2 to standard -``` +Identify components that can drop props now that context provides the data. -### 5.9 Type Cleanup +### 7.8 Type cleanup Ensure all TypeScript types reflect the final context shape. Remove any temporary types used during migration. -### 5.10 Documentation +### 7.9 Documentation -- [ ] Update inline comments if needed -- [ ] Remove any "TODO: remove after refactor" comments -- [ ] Update component documentation if it references old structure +- [ ] Update inline comments that describe the legacy/refactored split - `IIIFViewer/index.tsx` and the context barrel both carry explanatory comments that die with them, but others reference the split in passing +- [ ] Remove any "TODO: remove after itemViewerRefactor is fully rolled out" comments +- [ ] Update this RFC's status, and close out [#13273](https://github.com/wellcomecollection/wellcomecollection.org/issues/13273) items that only existed because of the split ## Success Criteria -- [ ] No more .legacy.tsx files -- [ ] No more .refactored.tsx files -- [ ] No ItemViewerContextV2 references -- [ ] Feature flag removed from toggles +- [ ] No `legacy/` directory under `IIIFViewer`, and no `legacy.tsx` context +- [ ] No `refactored/` directory - its contents sit directly under `IIIFViewer` +- [ ] Feature flag removed from `toggles.ts` and the toggles package redeployed +- [ ] One import path for the context, no flag-switching barrel +- [ ] No `isRefactoredContext`, and no migration console logs - [ ] All tests still pass - [ ] TypeScript compiles with no errors - [ ] Application runs correctly @@ -138,4 +125,4 @@ Ensure all TypeScript types reflect the final context shape. Remove any temporar The IIIF Viewer context refactoring is complete! **See also:** -- [14-risks-and-success.md](./14-risks-and-success.md) - Final success metrics +- [15-risks-and-success.md](./15-risks-and-success.md) - Final success metrics diff --git a/rfcs/086-item-viewer-refactor/README.md b/rfcs/086-item-viewer-refactor/README.md index 27794055..616c2a20 100644 --- a/rfcs/086-item-viewer-refactor/README.md +++ b/rfcs/086-item-viewer-refactor/README.md @@ -1,14 +1,26 @@ # RFC 086: IIIF Viewer Context Refactoring -**Status:** Not started +**Status:** Phases 0-6 complete. Phase 7 (cleanup) is blocked on the `itemViewerRefactor` toggle being defaulted to ON for 1+ week. **Estimated effort:** 14-17 hours -**Last modified:** 2026-04-14T13:30:00+00:00 +**Last modified:** 2026-09-15T10:31:29+00:00 ## Purpose This folder contains a comprehensive plan to refactor the IIIF Viewer context to eliminate code duplication and centralise derived state calculations. **Key principle:** Write automated tests BEFORE refactoring (test-first approach), then use manual testing for extra confidence. +## Names in these documents vs the code + +The phase documents were written before implementation, and use provisional names the code doesn't match. Translate as follows when following them: + +| In these documents | In the code | +|---|---| +| `contexts/ItemViewerContextV2/` | `content/webapp/contexts/ItemViewerContext/refactored.tsx`, with `legacy.tsx` beside it and `index.tsx` selecting between the two on the toggle | +| `Component.legacy.tsx` / `Component.refactored.tsx` | `IIIFViewer/legacy/Component.tsx` / `IIIFViewer/refactored/Component.tsx` - directories, not filename suffixes | +| `iiifViewerRefactored` toggle | `itemViewerRefactor` | + +`currentCanvasIndex` also never went onto the context: it would duplicate `query.canvas`, which is already there. Several documents list it as a context value - Phase 6 in particular is written on the assumption that it exists. + ## Table of Contents ### Understanding the Problem @@ -60,17 +72,23 @@ See [14-testing-strategy.md](./14-testing-strategy.md) for automated test requir 5. **Test-first workflow** - Green to Green refactoring (tests pass before and after) 6. **Manual tests as backup** - Comprehensive checklist for extra confidence 7. **Context for shared state only** - Only add to context if used by 2+ components or likely to be needed soon -8. **Hooks for complex logic** - Extract to custom hooks for testability, even if only used once8. **Split components with drastically different modes** - See [Future Improvements](./16-future-improvements.md) for details +8. **Hooks for complex logic** - Extract to custom hooks for testability, even if only used once +9. **Split components with drastically different modes** - See [Future Improvements](./16-future-improvements.md) for details + ## Progress Tracking -- [ ] Phase 0: Type Audit -- [ ] Phase 1: Feature Flag Setup -- [ ] Phase 2: Split MainViewer Components -- [ ] Phase 3: Canvas Data (with automated tests) -- [ ] Phase 4: Download Logic -- [ ] Phase 5: Restriction Status -- [ ] Phase 6: Duplicate Calls -- [ ] Phase 7: Cleanup +Tickets are in [wellcomecollection.org](https://github.com/wellcomecollection/wellcomecollection.org). + +- [x] Phase 0: Type Audit - [#12982](https://github.com/wellcomecollection/wellcomecollection.org/issues/12982) +- [x] Phase 1: Feature Flag Setup - [#12983](https://github.com/wellcomecollection/wellcomecollection.org/issues/12983) +- [x] Phase 2: Split MainViewer Components - [#12985](https://github.com/wellcomecollection/wellcomecollection.org/issues/12985) +- [x] Phase 3: Canvas Data (with automated tests) - [#12986](https://github.com/wellcomecollection/wellcomecollection.org/issues/12986) +- [x] Phase 4: Download Logic - [#12987](https://github.com/wellcomecollection/wellcomecollection.org/issues/12987) +- [x] Phase 5: Restriction Status - [#12988](https://github.com/wellcomecollection/wellcomecollection.org/issues/12988) +- [x] Phase 6: Duplicate Calls - [#12989](https://github.com/wellcomecollection/wellcomecollection.org/issues/12989) +- [ ] Phase 7: Cleanup - [#12990](https://github.com/wellcomecollection/wellcomecollection.org/issues/12990), waiting on the toggle being defaulted to ON + +Two tickets sit outside the phase structure: [#12984](https://github.com/wellcomecollection/wellcomecollection.org/issues/12984) (review and add tests) ran alongside Phases 0-2, and [#13329](https://github.com/wellcomecollection/wellcomecollection.org/issues/13329) (readability/normalisation pass) alongside Phase 3. Out-of-scope findings picked up along the way are collected in [#13273](https://github.com/wellcomecollection/wellcomecollection.org/issues/13273), which includes items to clear before the toggle can go on publicly. --- diff --git a/rfcs/README.md b/rfcs/README.md index 29173405..6a825885 100644 --- a/rfcs/README.md +++ b/rfcs/README.md @@ -74,6 +74,7 @@ _This is generated from the RFCs in this directory using `.scripts/create_table_ | RFC ID | Summary | Next Line | Last Modified | |--------|---------|-----------|---------------| +| [086-item-viewer-refactor](086-item-viewer-refactor/README.md) | RFC 086: IIIF Viewer Context Refactoring | This folder contains a comprehensive plan to refactor the IIIF Viewer context to eliminate code duplication and centralise derived state calculations. | 15 Sep 2026 | | [089-identifiers-api](089-identifiers-api/README.md) | RFC 089: Identifiers API | This RFC proposes a small, read-only **Identifiers API** that resolves a **canonical** catalogue identifier to its **source** identifier(s) and back, served from the catalogue ID Registry (the same store the ID Minter writes to, per [RFC 083](../083-stable_identifiers/README.md)). It provides that translation in one place, between the canonical ids the public surface uses and the source ids (Sierra numbers, FOLIO UUIDs, CALM/Axiell refs) that the underlying systems require across the Sierra/CALM → FOLIO/Axiell migration. It sets out the contract, the AWS architecture, the authentication and cost model, the caching strategy, and what a working prototype has already established. | 15 Sep 2026 | | [091-digitisation-ingest-identifiers](091-digitisation-ingest-identifiers/README.md) | RFC 091: Preservation identifiers across the LMS migration | Wellcome Collection is migrating its library management system from Sierra to Folio, and its archive management system from CALM to Axiell Collections. The Sierra b-number is embedded in storage locations, METS records, IIIF manifest URIs, and the join key that merges digitised content onto the public catalogue work. This RFC names that identifier role the **preservation identifier**, records the decision that preservation identifiers remain the canonical identifiers for digital objects (nothing is minted at ingest, and IIIF Manifest URIs do not move to the catalogue Work id), and sets out cross-migration and post-migration ingest paths covering both digitised and born-digital content. For items ingested after the migration the proposed preservation identifier is the Folio instance HRID, e.g. `in00012345`. | 18 Aug 2026 | | [092-digitised-archive-links](092-digitised-archive-links/README.md) | RFC 092: Keeping digitised archive material connected after Sierra | Digitised archive material on wellcomecollection.org is connected to its archive description through the CALM-synced Sierra bib, and those bibs are deliberately not migrated to FOLIO. This RFC records how the connection works today, shows that the catalogue pipeline can maintain it with no Sierra or FOLIO record existing provided the Axiell Collections record cites the Sierra b number, and quantifies the gap (roughly 93% of digitised archive records carry no b number in Axiell). The proposed fix is to import the missing b numbers into Axiell Collections, matched on each record's public reference and verified through the CALM RecordID carried in the migration. It is the archive-side companion to RFC 091, which covers the library-side (FOLIO-target) half of post-migration digitisation merging. | 18 Aug 2026 | @@ -81,7 +82,6 @@ _This is generated from the RFCs in this directory using `.scripts/create_table_ | [090-axiell-folio-sync](090-axiell-folio-sync/README.md) | RFC 090: CMS to LMS Sync | This RFC proposes an automated pipeline to synchronize data from **Axiell Collections (AxC)**, the new Content Management System (CMS), into **FOLIO**, the new Library Management System (LMS). The records from Axiell Collections need to exist in FOLIO so they can be requested and circulated in the LMS. It is designed for **idempotency** (safe to replay without duplication), **auditability** (every create / update / suppress is recorded), and **graceful error isolation** (one bad record never halts the batch). | 30 Jun 2026 | | [088-folio-identity-requesting-migration](088-folio-identity-requesting-migration/README.md) | RFC 088: Migrating identity, requesting and items APIs from Sierra to FOLIO | This RFC describes how we move the identity, requesting and item-availability APIs that power `wellcomecollection.org` from our current Library Management System (LMS), **Sierra**, to its replacement, **FOLIO**. It sets out the proposed architecture (a parallel, FOLIO-backed **v2** identity API fronted by Auth0), the embedded API contract, the migration plan (a per-request website toggle plus lazy patron migration, culminating in a single coordinated cutover), and the questions still open before cutover. | 26 Jun 2026 | | [087-kiosk-mode](087-kiosk-mode/README.md) | RFC 087: wellcomecollection.org in kiosk mode | This RFC serves to outline how we propose to offer in-venue experiences using our current website, while optimising it for a different experience than usual. | 13 May 2026 | -| [086-item-viewer-refactor](086-item-viewer-refactor/README.md) | RFC 086: IIIF Viewer Context Refactoring | This folder contains a comprehensive plan to refactor the IIIF Viewer context to eliminate code duplication and centralise derived state calculations. | 14 Apr 2026 | | [084-shopify-integration-strategies](084-shopify-integration-strategies/README.md) | RFC 084: Shopify Integration Approaches for Wellcome Collection | This research outlines five approaches for integrating Shopify with the Wellcome Collection website, ranging from simple embedded solutions to fully headless implementations. | 16 Feb 2026 | | [083-stable_identifiers](083-stable_identifiers/README.md) | RFC 083: Stable identifiers following mass record migration | This RFC discusses what will happen to public catalogue identifiers following the mass migration of records from CALM/Sierra to Axiell Collection / Folio and how we can update the catalogue pipeline to accommodate this change. | 10 Feb 2026 | | [082-curated-collections-prismic](082-curated-collections-prismic/README.md) | RFC 082: Curated Collections × Prismic | As we increasingly connect Collections to Content (Prismic), we are having a lot of conversations that can get muddled together. We thought it best to separate concerns. There are a few things to address, from the selection process to the actual solution implementation, and we wanted to start documenting them. | 26 Jan 2026 |