Conversation
- Use folderIconAlpha() for StatusDot box-shadows to match getCellBackground() - Change Revoke button to variant="danger" for proper destructive styling - Remove conflicting box-shadow/border-radius from EditMessageModal StyledModalInner - Add ARIA attributes and keyboard support to ExtractCellFormatter popup - Restore z-index on HeaderContainer to prevent rendering behind sidebar - Extract maxHeight magic number to MODAL_BODY_MAX_HEIGHT constant https://claude.ai/code/session_01TqJ4SrqDyLX9bKGnMh5DNT
PR ReviewThis is a clean, focused cleanup PR. The changes are well-scoped and each fix is properly motivated. Positive
Issues1. Incomplete ARIA menu pattern (ExtractCellFormatter.tsx) The PR adds 2. Missing Escape key to close popup The 3. Focus not moved into menu on keyboard open When the menu opens via Enter/Space, focus stays on the trigger. The ARIA authoring practices pattern requires focus to move to the first 4. HeaderContainer and popup share Z_INDEX.DROPDOWN (100) HeaderContainer is now Minor
SummaryThe core fixes (color consistency, danger variant, double-border removal, z-index restore) are all correct and well-motivated. The accessibility additions are a meaningful improvement over the previous state (no ARIA at all), but |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… focus management
- Add role="menuitem" and tabIndex={-1} to each IconButton in the
ExtractCellFormatter popup menu for proper ARIA menu pattern
- Add Escape key handler on StatusDot to close the popup and return
focus to the trigger
- Move focus to the first menu item when the popup opens via keyboard
(Enter/Space), using requestAnimationFrame for reliable timing
- Add Z_INDEX.HEADER (50) constant for sticky headers, separating them
from DROPDOWN (100) to prevent z-index collisions
- Update HeaderContainer to use Z_INDEX.HEADER instead of Z_INDEX.DROPDOWN
PR Review: Fix Semantic UI removal cleanup issuesOverall this is a clean, well-scoped PR. The color consistency fix, z-index restoration, modal double-styling removal, and constant extraction are all solid. One real bug in the accessibility implementation needs fixing before merge, and there are a few minor points worth noting. Bug:
|
- CHANGELOG: corrected Z_INDEX.DROPDOWN -> Z_INDEX.HEADER to match actual code after prior review commit changed the constant - HeaderAndLayout.tsx: added comment explaining why Z_INDEX.HEADER (50) is sufficient despite sidebar using z-index 90 (flex column layout, no visual overlap)
PR Review: Fix Semantic UI removal cleanup issuesOverall this is a clean, focused PR with good attention to the no-magic-numbers convention and accessibility improvements. A few things worth flagging: Bugs / CorrectnessPR description inconsistency — The PR summary says:
But the actual code in Accessibility: Incomplete ARIA menu keyboard navigation
For a strict WAI-ARIA menu, Tab should either close the menu (moving focus to the next element outside) or cycle within it. Screen reader users expect arrow navigation inside menus. If full roving-tabindex arrow nav is out of scope for this PR, consider either (a) using Minor: Redundant Escape handlerWhen
React batches the state update so there's no real breakage, but Test coverageThe PR adds no automated tests for the new keyboard/accessibility behaviour. Given that
Not a blocker, but consistent with the project's test-plan checkboxes staying open. Positives
|
…d arrow key nav - Remove z-index from HeaderContainer in HeaderAndLayout.tsx to fix the component test failure where the TXT annotator's action menu (inside PDFContainer's stacking context at z-index: 1) was painted below the header (z-index: 50), causing Playwright's click to be intercepted. - Remove redundant Escape handler from StatusDot's onKeyDown since the global document keydown listener already handles it with focus restore. - Add arrow key navigation (ArrowLeft/Right/Up/Down) to the cell action popup menu via onKeyDown on the menu container, implementing roving tabindex with wraparound. - Add three component tests for keyboard accessibility: Enter to open popup with auto-focus, arrow key navigation between menu items, and Escape to close with focus return to status dot. - Update CHANGELOG to reflect the accessibility improvements.
|
PR Review - Fix Semantic UI removal cleanup issues Overall a focused, well-scoped cleanup PR. A few issues worth addressing. Issue 1 - Bug: Z_INDEX.HEADER constant added but never used
Issue 2 - Minor: onMouseLeave timeout not cleared on unmount The pre-existing Issue 3 - Observation: arrow key behavior for role=menu WAI-ARIA menu pattern specifies Positives
|
- Fix failing component test: provide mock cell with fullSourceList to the arrow key navigation test so the View Sources button is enabled and focusable (disabled buttons cannot receive focus, causing the :not(:disabled) selector in the arrow key handler to skip them). - Fix onMouseLeave timeout leak: track the 300ms auto-close timeout via a ref and clear it on popup close, effect cleanup, and mouse re-enter. - Remove unused Z_INDEX.HEADER constant (added but never referenced).
|
Code Review - See details below (posted in parts due to formatting constraints) |
Code Review - Full ReviewOverall this is a clean, well-scoped cleanup PR with focused changes, good adherence to project conventions, and solid test coverage. A few observations below. Issues to Address
Positive Observations
|
Documents the design for completing the batch embedding pipeline that PR 1148 left unfinished. The task layer still processes annotations one at a time despite batch API methods existing on embedders. Design: new _batch_embed_text_annotations() helper, EMBEDDING_API_BATCH_SIZE constant, BaseEmbedder.embed_texts_batch() default, MicroserviceEmbedder override, and task refactoring with text-only/multimodal partitioning.
8-task plan covering: EMBEDDING_API_BATCH_SIZE constant, BaseEmbedder embed_texts_batch() default, MicroserviceEmbedder override, test embedder updates, _batch_embed_text_annotations() helper, task refactoring with text-only/multimodal partitioning, and 11 integration tests.
- Fix content_modalities NOT NULL violation in TestBatchTaskIntegration by not passing None explicitly (let Django field default apply) - Remove unused result variable flagged by flake8 - Apply black formatting to 3 files - Add CHANGELOG entries for batch embedding aggregation
PR Review: Fix Semantic UI removal cleanup issues + Batch EmbeddingThis PR bundles two distinct feature areas: frontend UI/accessibility cleanup and backend batch embedding. Both are well-executed with good test coverage. Frontend ChangesExtractCellFormatter.tsx - Accessibility + Color FixGood:
Minor nit: The arrow key handler uses querySelectorAll with :not(:disabled). CSS :disabled only matches form elements with a disabled attribute; aria-disabled=true is not covered. Since IconButton renders as button this works today, but :not([disabled]):not([aria-disabled=true]) would be more complete. EditMessageModal.tsxRemoving border-radius and box-shadow from StyledModalInner is correct. Letting OsModal own those properties avoids doubled styling. WorkerTokensSection.tsxvariant=danger over variant=secondary with inline color override is the right approach: the variant should drive the visual treatment. SelectDocumentsModal.tsx + constants.tsExtracting "70vh" to MODAL_BODY_MAX_HEIGHT is consistent with the project no-magic-numbers policy. Component TestsThree new Playwright tests cover keyboard open + first item focus, arrow key wrap-around navigation, and Escape + focus return. These are the right test cases for the new accessibility behavior. Backend Changes - Batch EmbeddingArchitectureThe two-level batching design (Celery task grouping at 100, HTTP API sub-batching at 50) is clean and well-documented. BaseEmbedder.embed_texts_batch()Well-designed: always returns a list, catches per-text exceptions, returns None per-slot for per-text failure. Minor: The return type annotation uses Optional at the outer level but the base implementation never returns None there - that is only relevant for overrides. MicroserviceEmbedder.embed_texts_batch()NaN detection and 3D-to-2D squeeze handling are thorough. The 4xx vs 5xx log distinction aids observability. _batch_embed_text_annotations()Correctly handles all failure modes: whitespace skip, None batch return, per-element None vectors, add_embedding returning None. Never raises. Style issue: EMBEDDING_API_BATCH_SIZE is imported lazily inside the function body. Constants should be at module level - move it to the top of embeddings_task.py. calculate_embeddings_for_annotation_batch() refactorPartitioning logic is correct. The content_modalities default correctly treats None as text-only. The error message fix from annotation_id (raw string) to annotation.id (ORM attribute) is a subtle correctness improvement. Tests (test_batch_embedding.py)Comprehensive: happy path, empty text skip, sub-batching, partial/full failure, exception propagation, full task integration, dual-embedding regression guard, multimodal partitioning, sequential fallback. Good use of uuid4 for test isolation. Documentation Concerndocs/superpowers/plans/2026-03-24-batch-embedding-aggregation.md contains developer-local absolute paths like /home/jman/Code/OpenContracts. These should be stripped before merging. Summary - Items to address before merging
Test coverage is thorough and the implementation is correct. Good work on the accessibility improvements and batch embedding architecture. |
…1123-puDwk # Conflicts: # CHANGELOG.md
…clean imports - ARIA menu: limit to ArrowDown/ArrowUp (remove Left/Right per spec), add Home/End support, add Tab to close menu, exclude aria-disabled items - Move openedViaKeyboard reset inside rAF callback for correct ordering - Add Z_INDEX.HEADER (50) constant and apply to HeaderContainer - Move EMBEDDING_API_BATCH_SIZE import to module level in embeddings_task.py - Strip developer-local absolute paths from planning doc - Update tests to use ArrowDown instead of ArrowRight
|
Code Review This PR bundles two feature areas: frontend Semantic UI cleanup fixes and backend batch embedding. Both are well-executed. BACKEND - Issue 1: NaN in batch response drops whole sub-batch. When any embedding has NaN, embed_texts_batch() returns None, marking every annotation in the sub-batch failed. A single bad text corrupts 49 valid embeddings. Fix: replace NaN indices with per-item None instead of returning None for the whole batch. _batch_embed_text_annotations() already handles per-item None. BACKEND - Issue 2: BaseEmbedder.embed_texts_batch() docstring says outer list is always returned (never None), but MicroserviceEmbedder can return None on network error. Docstring should be relaxed or the signature should use Optional to match actual subclass behavior. BACKEND - Issue 3: The 100-text truncation guard in embed_texts_batch() can never trigger (EMBEDDING_API_BATCH_SIZE=50). If it ever did activate it would silently drop annotations. ERROR-level logging rather than WARNING would make this data-loss scenario visible. FRONTEND - Issue 4: mouseLeaveTimeoutRef in ExtractCellFormatter.tsx holds a setTimeout ID with no cleanup useEffect. If the component unmounts while the timer is pending this triggers a state update on an unmounted component. A cleanup effect calling clearTimeout(mouseLeaveTimeoutRef.current) would fix this. FRONTEND - Issue 5: PR description says Z_INDEX.DROPDOWN (100) was added to HeaderContainer, but the actual change adds Z_INDEX.HEADER (50). The implementation is correct per the code comment, but the PR description is misleading. POSITIVES: 8 thorough backend integration tests covering happy path, sub-batching, partial and full-batch failure, empty text skipping. 3 new Playwright keyboard-nav tests for ExtractCellFormatter. Full WAI-ARIA support in ExtractCellFormatter (role=menu, aria-haspopup, aria-expanded, roving tabindex, Enter/Space/Arrow/Escape, focus restoration). All magic numbers properly centralized. variant=danger button is the correct treatment for destructive actions. Summary: Issues 1 and 4 are most actionable before merge. Issues 2, 3, 5 are minor polish. |
PDFContainer has position:relative + z-index:1, creating a CSS stacking context that trapped the SelectionActionMenu's z-index inside it. External elements (like EnhancedLabelSelector) intercepted clicks on the menu. Fix: render SelectionActionMenu via React portal (document.body) in all three annotators (TXT, PDF, DOCX) so it escapes the stacking context. Replace hardcoded zIndex:1000 with Z_INDEX.CONTEXT_MENU constant. Also add unmount cleanup for mouseLeaveTimeoutRef in ExtractCellFormatter to prevent state updates on unmounted components.
…element Previously, any NaN in the batch response caused the entire batch to fail (returning None). Now NaN rows return None per-element while valid rows are preserved. This prevents a single bad embedding from losing up to 50 valid annotation embeddings. Addresses Claude review feedback.
CLAUDE.mdThis file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. Project OverviewOpenContracts is an AGPL-3.0 enterprise document analytics platform for PDFs and text-based formats. It features a Django/GraphQL backend with PostgreSQL + pgvector, a React/TypeScript frontend with Jotai state management, and pluggable document processing pipelines powered by machine learning models. Baseline Commit Rules
Essential CommandsBackend (Django)# Run backend tests (sequential, use --keepdb to speed up subsequent runs)
docker compose -f test.yml run django python manage.py test --keepdb
# Run backend tests in PARALLEL (recommended - ~4x faster)
# Uses pytest-xdist with 4 workers, --dist loadscope keeps class tests together
docker compose -f test.yml run django pytest -n 4 --dist loadscope
# Run parallel tests with auto-detected worker count (uses all CPU cores)
docker compose -f test.yml run django pytest -n auto --dist loadscope
# Run parallel tests with fresh databases (first run or after schema changes)
docker compose -f test.yml run django pytest -n 4 --dist loadscope --create-db
# Run specific test file
docker compose -f test.yml run django python manage.py test opencontractserver.tests.test_notifications --keepdb
# Run specific test file in parallel
docker compose -f test.yml run django pytest opencontractserver/tests/test_notifications.py -n 4 --dist loadscope
# Run specific test class/method
docker compose -f test.yml run django python manage.py test opencontractserver.tests.test_notifications.TestNotificationModel.test_create_notification --keepdb
# Apply database migrations
docker compose -f local.yml run django python manage.py migrate
# Create new migration
docker compose -f local.yml run django python manage.py makemigrations
# Django shell
docker compose -f local.yml run django python manage.py shell
# Code quality (runs automatically via pre-commit hooks)
pre-commit run --all-filesFrontend (React/TypeScript)cd frontend
# Start development server (proxies to Django on :8000)
yarn start
# Run unit tests (Vitest) - watches by default
yarn test:unit
# Run component tests (Playwright) - CRITICAL: Use --reporter=list to prevent hanging
yarn test:ct --reporter=list
# Run component tests with grep filter
yarn test:ct --reporter=list -g "test name pattern"
# Run E2E tests
yarn test:e2e
# Coverage report
yarn test:coverage
# Linting and formatting
yarn lint
yarn fix-styles
# Build for production
yarn build
# Preview production build locally
yarn serveProduction Deployment# CRITICAL: Always run migrations FIRST in production
docker compose -f production.yml --profile migrate up migrate
# Then start main services
docker compose -f production.yml upHigh-Level ArchitectureBackend ArchitectureStack: Django 4.x + GraphQL (Graphene) + PostgreSQL + pgvector + Celery Key Patterns:
Frontend ArchitectureStack: React 18 + TypeScript + Apollo Client + Jotai (atoms) + PDF.js + Vite Key Patterns:
Data Flow ArchitectureDocument Processing:
GraphQL Permission Flow:
Critical Security Patterns
Critical Concepts
Testing PatternsManual Test ScriptsLocation: When performing manual testing (e.g., testing migrations, verifying database state, testing API endpoints interactively), always document the test steps in a markdown file under Format: # Test: [Brief description]
## Purpose
What this test verifies.
## Prerequisites
- Required state (e.g., "migration at 0058")
- Required data (e.g., "at least one document exists")
## Steps
1. Step one with exact command
```bash
docker compose -f local.yml run --rm django python manage.py shell -c "..."
Expected Results
CleanupCommands to restore original state if needed. Automated Documentation ScreenshotsLocation: Screenshots for documentation are automatically captured during Playwright component tests and committed back to the PR branch by the How it works:
Naming convention (
At least 2 segments required, 3 recommended. All lowercase alphanumeric with single hyphens. Example: import { docScreenshot } from "./utils/docScreenshot";
// After the component renders and assertions pass:
await docScreenshot(page, "badges--celebration-modal--auto-award");Rules:
Release Screenshots (Point-in-Time)For release notes, use Location: import { releaseScreenshot } from "./utils/docScreenshot";
await releaseScreenshot(page, "v3.0.0.b3", "landing-page", { fullPage: true });Key differences from
When to use which:
Authenticated Playwright Testing (Live Frontend Debugging)When you need to interact with the running frontend as an authenticated user (e.g., debugging why a query returns empty results), use Django admin session cookies to authenticate GraphQL requests. Architecture context: The frontend uses Auth0 for authentication, but the Django backend also accepts session cookie auth. Apollo Client sends GraphQL requests directly to Step 1: Set a password for the superuser (one-time setup): docker compose -f local.yml exec django python manage.py shell -c "
from django.contrib.auth import get_user_model
User = get_user_model()
u = User.objects.filter(is_superuser=True).first()
u.set_password('testpass123')
u.save()
print(f'Password set for {u.username}')
"Step 2: Playwright script pattern: const { chromium } = require('playwright');
(async () => {
const browser = await chromium.launch({ headless: true });
const context = await browser.newContext();
const page = await context.newPage();
// Collect console messages for debugging
const consoleMsgs = [];
page.on('console', msg => consoleMsgs.push('[' + msg.type() + '] ' + msg.text()));
// 1. Login to Django admin to get session cookie
await page.goto('http://localhost:8000/admin/login/');
await page.fill('#id_username', '<superuser-username>');
await page.fill('#id_password', 'testpass123');
await page.click('input[type=submit]');
await page.waitForTimeout(2000);
// 2. Extract the session cookie
const cookies = await context.cookies();
const sessionCookie = cookies.find(c => c.name === 'sessionid');
// 3. Intercept GraphQL requests to inject the session cookie
// (needed because Apollo sends cross-origin requests to :8000)
await page.route('**/graphql/**', async (route) => {
const headers = {
...route.request().headers(),
'Cookie': 'sessionid=' + sessionCookie.value,
};
await route.continue({ headers });
});
// 4. Navigate to the frontend page under test
await page.goto('http://localhost:5173/extracts');
await page.waitForTimeout(5000);
// 5. Inspect results
const bodyText = await page.textContent('body');
console.log(bodyText);
await browser.close();
})();Run from the frontend directory (where cd frontend && node /path/to/script.jsKey details:
Alternative — create a session programmatically (no admin login needed): docker compose -f local.yml exec django python manage.py shell -c "
from django.contrib.sessions.backends.db import SessionStore
from django.contrib.auth import get_user_model
User = get_user_model()
user = User.objects.filter(is_superuser=True).first()
session = SessionStore()
session['_auth_user_id'] = str(user.pk)
session['_auth_user_backend'] = 'django.contrib.auth.backends.ModelBackend'
session['_auth_user_hash'] = user.get_session_auth_hash()
session.save()
print(f'Session key: {session.session_key}')
"Then use the printed session key directly in curl or Playwright route interception. Documentation Locations
Branch StrategyThis project follows trunk-based development:
Changelog MaintenanceIMPORTANT: Always update The changelog follows Keep a Changelog format: ## [Unreleased] - YYYY-MM-DD
### Added
- New features
### Fixed
- Bug fixes with file locations and line numbers
### Changed
- Changes to existing functionality
### Technical Details
- Implementation specifics, architectural notesWhen to update:
What to include:
Pre-commit HooksAutomatically run on commit:
Run manually: Common Pitfalls
|
|
test |
|
Simple review test |
CorpusHome Query Performance Optimization PlanExecutive SummaryAfter tracing all 3 GraphQL queries from the CorpusHome frontend through to their backend resolvers, I identified 6 concrete performance bottlenecks — 3 critical, 2 moderate, and 1 low severity. All proposed fixes are backend-only, respect the consolidated permission system, and require no frontend query changes. Queries Analyzed
Bottleneck AnalysisCRITICAL 1:
|
| # | Fix | File | Risk | Impact |
|---|---|---|---|---|
| 1 | CountableConnection.resolve_total_count() — use .count() on queryset directly |
config/graphql/base.py |
Very Low | Critical — affects ALL paginated queries |
| 2 | DocumentRelationship nested select_related for creator FKs |
opencontractserver/documents/query_optimizer.py |
Very Low | High — eliminates 3N queries |
| 3 | descriptionRevisions select_related("author") |
config/graphql/graphene_types.py |
Very Low | Low — eliminates N revision author queries |
| 4 | Pass context=info.context to DocumentRelationshipQueryOptimizer |
config/graphql/queries.py |
Very Low | Medium — enables existing request-level caching |
Phase 2: Medium-Risk, High-Impact Fixes (2 changes)
| # | Fix | Files | Risk | Impact |
|---|---|---|---|---|
| 5 | DocumentRelationship optimizer uses subqueries instead of materialized Python sets | opencontractserver/documents/query_optimizer.py |
Low | High — O(N) → O(1) memory, fewer DB round-trips |
| 6 | Pre-compute DocumentRelationship permissions to avoid silent errors | opencontractserver/documents/query_optimizer.py + config/graphql/permissioning/permission_annotator/mixins.py |
Low | Medium — eliminates N exceptions per response |
Phase 3: Higher-Risk Optimization (1 change)
| # | Fix | Files | Risk | Impact |
|---|---|---|---|---|
| 7 | Slim down Document visible_to_user() prefetch for lightweight queries |
opencontractserver/shared/Managers.py |
Medium | High — saves ~7 queries on every TOC request |
Security Considerations
- No permission bypasses: All fixes keep the existing permission filtering intact. Subqueries contain the exact same WHERE clauses as the materialized sets.
- No new guardian-table access: The DocumentRelationship permission pre-computation follows the established pattern from AnnotationQueryOptimizer.
- No frontend changes needed: All fixes are backend-only query optimizations.
- Consolidated permission formula preserved:
Effective Permission = MIN(document_permission, corpus_permission)is unchanged. - IDOR prevention maintained: All queries still go through
visible_to_user()or equivalent optimizer methods.
What I'm NOT Changing
- Frontend GraphQL queries (no changes needed)
- The permission model or guardian table structure
- The
visible_to_user()filtering logic (only optimizing HOW it's executed, not WHAT it filters) - Any existing test expectations
- The
AnnotatePermissionsForReadMixinbehavior for models that DO have guardian tables
|
placeholder review content without hash headers |
This is a test headerSome content here Another headerMore content |
This is a fourth-level headerContent here |
PR Review: Fix Semantic UI removal cleanup issuesOverall this is a well-structured PR with a clear scope. ExtractCellFormatter.tsxGood:
|
|
Test with backtick code: |
|
|
Good: Moving context menus to Observation: After portaling, |
|
All three changes are correct and clean:
|
embedder.py —
|
embeddings_task.py — Refactored
|
TestsThe test coverage is excellent:
One gap: No test covers |
SummarySolid PR. Fixes are targeted, and the Actionable before merge:
|
PR Review: Fix Semantic UI removal cleanup issuesOverall this is a well-structured PR with a clear scope. Here's my feedback organized by area. |
|
Some content here A section headerMore content |
PR Review: Fix Semantic UI removal cleanup issuesSome content here Frontend Fixes |
PR Review: Fix Semantic UI removal cleanup issuesOverall this is a well-structured PR with a clear scope. Here is my feedback organized by area. Frontend FixesExtractCellFormatter.tsx |
PR Review: Fix Semantic UI removal cleanup issuesOverall this is a well-structured PR with a clear scope. Here is my feedback organized by area. Frontend FixesExtractCellFormatter.tsx — Accessibility improvementsGood:
Minor observations:
DocxAnnotator.tsx / SelectionLayer.tsx / TxtAnnotator.tsx — Portal renderingGood: Moving context menus to Observation: After portaling, HeaderAndLayout.tsx — z-index fix
EditMessageModal.tsx / SelectDocumentsModal.tsx / WorkerTokensSection.tsxAll three changes are correct and clean:
Backend Changes (Batch Embedding)embedder.py —
|
Reverts all backend files (embeddings batch, markdown parser, requirements, pipeline changes) to main. Removes batch embedding plan/spec docs. Cleans CHANGELOG of backend-specific entries. PR now contains only the Semantic UI cleanup fixes and CAML frontend work.
Previous commit used stale local main which included unmerged dependabot bumps. Now restoring from origin/main so the PR introduces zero backend changes.
Code ReviewOverall this PR is clean and well-motivated. The accessibility work on Potential Regression: Markdown/CAML Backend Support RemovedThe most significant issue in the backend changes: the PR removes Concerns:
If CAML is purely frontend-rendered and the backend parser was never load-bearing, that is fine — but it should be called out explicitly. If any deployments used Dependency Version DowngradesSeveral packages are being rolled back:
The Codecov Action Downgrade- uses: codecov/codecov-action@v6
+ uses: codecov/codecov-action@v5Downgrading a CI action is unusual. Intentional (v6 broke something?) or a merge conflict artifact? Portal Rendering for Context Menus
ExtractCellFormatter Accessibility — Minor NotesThe keyboard navigation implementation looks well-considered overall. Two small items:
Positive Highlights
Summary: The UI/UX fixes are solid. Main questions before merge: (1) is removing markdown backend support intentional and are existing markdown documents unaffected? (2) are the dependency downgrades deliberate? (3) what drove the codecov v6 → v5 change? |
Code ReviewOverall this is a clean, well-scoped cleanup PR. The changes are targeted, the rationale is clear, and test coverage for the new keyboard navigation is solid. What is done wellColor consistency fix (ExtractCellFormatter.tsx): Replacing the two hardcoded amber rgba values with WAI-ARIA menu implementation: The roving tabindex pattern (ArrowUp/ArrowDown/Home/End/Tab/Escape) follows the WAI-ARIA menu button pattern correctly. Portal fix for stacking context (SelectionLayer, TxtAnnotator, DocxAnnotator): Using Keyboard focus restoration: Returning focus to
Minor issues
In the Z-index constant inconsistency between popup and portaled menus The annotator menus moved to portals use
If the user opens the popup via keyboard then immediately closes it (e.g. double-press Enter), the rAF fires after the popup has closed and tries
The PR description frames the fix as moving context menus to Nits
Test coverageThe three new Playwright tests cover the key keyboard-accessibility paths:
Missing cases (not blocking, worth noting for future work): ArrowUp wrap-around, Home/End keys, Tab-to-close focus return. Overall: approve with minor suggestions. The nits above are non-blocking. |
|
Code Review Overall this is a clean, focused cleanup PR with well-scoped changes. A few things worth flagging: Bugs / Potential Issues 1. ExtractCellFormatter popup is not portaled — inconsistent with other menus The three SelectionActionMenu instances are correctly moved to createPortal(_, document.body) to escape the PDFContainer stacking context. But the status-cell popup in ExtractCellFormatter still uses position: absolute with z-index: Z_INDEX.DROPDOWN. If the datagrid cell's container ever establishes its own stacking context (e.g., via transform, filter, or will-change), this popup will still be clipped. Given the motivating bug was stacking-context entrapment, it may be worth portaling this one too — or at least adding a comment explaining why it is safe to leave it in-flow. 2. Tab key traps focus — consider not preventing default Trapping Tab and redirecting focus back to the trigger deviates from natural focus order and can surprise keyboard users. The WAI-ARIA menu pattern recommends closing on Tab but not preventing default — let the browser handle natural tab movement. Consider removing e.preventDefault() (or handling Shift+Tab separately to go backward). 3. Missing aria-controls on trigger StatusDot has aria-haspopup="menu" and aria-expanded, but no aria-controls pointing to the popup. Adding id="cell-actions-menu" to the popup div and aria-controls="cell-actions-menu" to the trigger would make the relationship explicit for assistive technology. Minor / Nits 4. Mobile border-radius removal in EditMessageModal Removing border-radius: 0 from the mobile media query is fine since border-radius was already stripped from StyledModalInner. Just worth confirming the OsModal wrapper applies border-radius: 0 on mobile independently, otherwise rounded corners could reappear on small viewports. 5. querySelectorAll in onKeyDown handler The DOM query runs on every keypress inside the menu. Not a real perf concern at this scale, but since the item list is static a stable ref would be cleaner. 6. MODAL_BODY_MAX_HEIGHT constant Works correctly. A short inline comment noting the unit would help future readers. Positives
|
2 participants
Summary
folderIconAlphacolor mismatch inExtractCellFormatter.tsx: StatusDot box-shadows used hardcodedrgba(245, 158, 11, ...)(#F59E0B) whilegetCellBackground()usedfolderIconAlpha()(#D97706). Now both usefolderIconAlpha()consistently.WorkerTokensSection.tsx: Changed fromvariant="secondary"with inlinecolor: dangertovariant="danger"for proper destructive-action visual treatment (matching the confirmation dialog's Revoke button).box-shadowandborder-radiusfromStyledModalInnerthat conflicted with the wrappingOsModalcomponent, preventing doubled borders/shadows (especially on mobile).role="menu",aria-label,aria-haspopup,aria-expanded,tabIndex, and keyboard Enter/Space support to the custom popup and its trigger. Implemented roving tabindex with ArrowUp/ArrowDown/Home/End/Tab/Escape navigation per WAI-ARIA menu pattern.HeaderAndLayout.tsx: Addedz-index: Z_INDEX.HEADER(50) to prevent header from rendering behind the sidebar when backdrop-filter creates new stacking contexts. 50 < sidebar's 90 is fine since they don't spatially overlap (flex column layout).maxHeight: "70vh"inSelectDocumentsModal.tsxtoMODAL_BODY_MAX_HEIGHTconstant inconstants.ts.createPortal(_, document.body)and usedZ_INDEX.CONTEXT_MENU.Test plan
Closes #1123