New UI components for the S3 explorer with Codex#1058
New UI components for the S3 explorer with Codex#1058marchufschmitt wants to merge 5 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a large S3 path UI (S3PathControl) with atom and integration stories, a set of bookmarks UI components and stories (chips, row items, pinned bar, entry list, modal), an S3 profile selector with stories and types, a utility and types for bookmarks, a small placeholder navigation file, and a VSCode MCP config file. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as S3PathControl (UI)
participant Validator as validatePath (async)
participant App as Host App / Callbacks
rect rgba(200,220,255,0.5)
User->>UI: edit path / click validate
UI->>Validator: call validatePath(draft)
Validator-->>UI: validation result (success / error / loading)
UI->>App: onNavigate or onError callbacks
App-->>UI: optional navigation ACK / state update
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.vscode/mcp.json (1)
2-8: Consider making this VSCode MCP config opt-in for contributors.This hardcodes a local endpoint (
127.0.0.1:3845) in a repo-tracked.vscodefile, which can be noisy/non-functional for teammates who don’t run that local MCP server. Prefer documenting it inREADME(or committing a sample file) so local workspace setup remains explicit and portable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vscode/mcp.json around lines 2 - 8, The .vscode/mcp.json currently hardcodes a local MCP endpoint under the "servers" -> "figma" -> "url" key which can be noisy for contributors; change this to an opt-in setup by removing or replacing the concrete URL in .vscode/mcp.json and instead commit a template (e.g., .vscode/mcp.json.sample) with a placeholder for "url" and add the real .vscode/mcp.json to .gitignore, and update README to document how to enable/configure the "figma" MCP server and the expected "servers" / "inputs" keys so contributors can opt-in safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.vscode/mcp.json:
- Around line 2-8: The .vscode/mcp.json currently hardcodes a local MCP endpoint
under the "servers" -> "figma" -> "url" key which can be noisy for contributors;
change this to an opt-in setup by removing or replacing the concrete URL in
.vscode/mcp.json and instead commit a template (e.g., .vscode/mcp.json.sample)
with a placeholder for "url" and add the real .vscode/mcp.json to .gitignore,
and update README to document how to enable/configure the "figma" MCP server and
the expected "servers" / "inputs" keys so contributors can opt-in safely.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/ui/shared/S3PathControl.atoms.stories.tsx`:
- Around line 14-17: The BackButton story is failing TS2322 because
StoryObj<typeof meta> derives props from meta.component (S3PathBackButton) which
requires classes, cx, canGoBack, and onBack but the story's args only provides
canGoBack; fix by giving the story a custom args type that relaxes or narrows
the component props (for example use StoryObj<React.ComponentProps<typeof
S3PathBackButton>> with Partial or pick only canGoBack) so the args: {
canGoBack: true } type-checks; update the BackButton story declaration to use
that custom StoryObj type (referencing meta, S3PathBackButton, and the
BackButton story name) and/or explicitly type args as Partial<...> to satisfy
strict TypeScript.
In `@web/src/ui/shared/S3PathControl.tsx`:
- Around line 1081-1088: The current copy logic calls
setInfoMessage(copyMessage) and onCopy(value) regardless of whether
navigator.clipboard?.writeText(value) succeeds; change the flow in S3PathControl
(the block using navigator.clipboard?.writeText, setInfoMessage and onCopy) so
that setInfoMessage(copyMessage) and onCopy?.(value) are executed only inside
the try after await navigator.clipboard?.writeText(value) completes
successfully, and keep the catch block for silently ignoring or optionally
setting an error message; this ensures "Path copied." is only shown when the
clipboard write actually succeeds.
- Line 1067: The forEach callback on tailIndices is using a single-expression
arrow which implicitly returns the result of collapsed.push(crumbs[index]) and
triggers the lint rule; change the callback to a block-bodied arrow (e.g.,
tailIndices.forEach(index => { collapsed.push(crumbs[index]); });) so the push
call is executed without an implicit return. Ensure you update the invocation
where tailIndices, collapsed, and crumbs are used in S3PathControl to use the
braced callback form.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/src/ui/shared/S3ExplorerNavigationBar.tsxweb/src/ui/shared/S3PathControl.atoms.stories.tsxweb/src/ui/shared/S3PathControl.stories.tsxweb/src/ui/shared/S3PathControl.tsx
✅ Files skipped from review due to trivial changes (1)
- web/src/ui/shared/S3ExplorerNavigationBar.tsx
| const meta = { | ||
| title: "Shared/S3PathControl/Atoms", | ||
| component: S3PathBackButton | ||
| } satisfies Meta<typeof S3PathBackButton>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "S3PathControl.atoms.stories.tsx" | head -20Repository: InseeFrLab/onyxia
Length of output: 112
🏁 Script executed:
cat -n ./web/src/ui/shared/S3PathControl.atoms.stories.tsxRepository: InseeFrLab/onyxia
Length of output: 9853
🏁 Script executed:
find . -type f -name "S3PathControl.ts*" -o -name "S3PathControl.tsx" | grep -v storiesRepository: InseeFrLab/onyxia
Length of output: 98
🏁 Script executed:
cat -n ./web/src/ui/shared/S3PathControl.tsxRepository: InseeFrLab/onyxia
Length of output: 50373
🏁 Script executed:
find . -name "tsconfig*" -type f | head -5Repository: InseeFrLab/onyxia
Length of output: 133
🏁 Script executed:
cat ./web/tsconfig.json | head -50Repository: InseeFrLab/onyxia
Length of output: 821
🏁 Script executed:
rg "TS2322" . --type-list 2>/dev/null | head -20Repository: InseeFrLab/onyxia
Length of output: 43
🏁 Script executed:
head -100 ./web/src/ui/shared/S3PathControl.atoms.stories.tsx | tail -60Repository: InseeFrLab/onyxia
Length of output: 1503
🏁 Script executed:
find . -type f -name "*.log" -o -name ".github" -type d | grep -E "(workflow|action|log)" | head -20Repository: InseeFrLab/onyxia
Length of output: 43
🏁 Script executed:
find .github -type f -name "*.yml" -o -name "*.yaml" 2>/dev/null | head -10Repository: InseeFrLab/onyxia
Length of output: 133
🏁 Script executed:
cat .github/workflows/ci.yaml | grep -A 20 -B 5 "typescript\|tsc\|type"Repository: InseeFrLab/onyxia
Length of output: 1161
🏁 Script executed:
cat .github/workflows/ci.yaml | head -150Repository: InseeFrLab/onyxia
Length of output: 4742
🏁 Script executed:
cat ./web/package.json | grep -A 5 '"build"'Repository: InseeFrLab/onyxia
Length of output: 414
🏁 Script executed:
cd ./web && yarn tsc --noEmit 2>&1 | grep -A 5 "S3PathControl.atoms.stories"Repository: InseeFrLab/onyxia
Length of output: 43
🏁 Script executed:
cd ./web && yarn tsc --noEmit 2>&1 | head -100Repository: InseeFrLab/onyxia
Length of output: 491
🏁 Script executed:
find ./web/src -name "*.stories.tsx" -o -name "*.stories.ts" | head -5Repository: InseeFrLab/onyxia
Length of output: 350
Fix TS2322 in BackButton story typing (blocks build with strict TypeScript).
BackButton uses StoryObj<typeof meta> where meta.component is S3PathBackButton, which requires props classes, cx, canGoBack, and onBack. The args: { canGoBack: true } at line 32 omits required props, causing a type mismatch despite the render function providing them. Storybook enforces args type compatibility at compilation time.
Suggested fix (custom args type for this story)
type Story = StoryObj<typeof meta>;
+type BackButtonStoryArgs = { canGoBack: boolean };
+type BackButtonStory = StoryObj<BackButtonStoryArgs>;
-export const BackButton: Story = {
+export const BackButton: BackButtonStory = {
args: {
canGoBack: true
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/S3PathControl.atoms.stories.tsx` around lines 14 - 17, The
BackButton story is failing TS2322 because StoryObj<typeof meta> derives props
from meta.component (S3PathBackButton) which requires classes, cx, canGoBack,
and onBack but the story's args only provides canGoBack; fix by giving the story
a custom args type that relaxes or narrows the component props (for example use
StoryObj<React.ComponentProps<typeof S3PathBackButton>> with Partial or pick
only canGoBack) so the args: { canGoBack: true } type-checks; update the
BackButton story declaration to use that custom StoryObj type (referencing meta,
S3PathBackButton, and the BackButton story name) and/or explicitly type args as
Partial<...> to satisfy strict TypeScript.
| collapsed.push(ellipsisCrumb); | ||
| } | ||
|
|
||
| tailIndices.forEach(index => collapsed.push(crumbs[index])); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "S3PathControl.tsx" -type fRepository: InseeFrLab/onyxia
Length of output: 98
🏁 Script executed:
head -1070 web/src/ui/shared/S3PathControl.tsx | tail -10Repository: InseeFrLab/onyxia
Length of output: 352
🏁 Script executed:
# Get exact content around line 1067
sed -n '1060,1075p' web/src/ui/shared/S3PathControl.tsxRepository: InseeFrLab/onyxia
Length of output: 521
🏁 Script executed:
# Check if there's a Biome/linter configuration
find . -name "biome.json" -o -name ".biomerc*" -o -name "biome.jsonc" | head -5Repository: InseeFrLab/onyxia
Length of output: 73
🏁 Script executed:
# Search for similar forEach patterns in the codebase to see if this rule is being enforced
rg "forEach\s*\(\s*\w+\s*=>\s*\w+\." -t tsx -t ts --max-count=10Repository: InseeFrLab/onyxia
Length of output: 88
🏁 Script executed:
# Check Biome configuration
cat biome.jsonRepository: InseeFrLab/onyxia
Length of output: 1835
🏁 Script executed:
# Search for similar forEach patterns without file type restriction
rg "forEach\s*\(\s*\w+\s*=>\s*\w+\." --max-count=10Repository: InseeFrLab/onyxia
Length of output: 165
🏁 Script executed:
# Verify the exact line number
rg -n "tailIndices.forEach" web/src/ui/shared/S3PathControl.tsxRepository: InseeFrLab/onyxia
Length of output: 134
🏁 Script executed:
# Get broader context to understand the full function
sed -n '1050,1080p' web/src/ui/shared/S3PathControl.tsxRepository: InseeFrLab/onyxia
Length of output: 860
Fix Biome lint error: forEach callback should not implicitly return.
At line 1067, collapsed.push(...) is returned implicitly from the arrow callback, which violates lint/suspicious/useIterableCallbackReturn. The single-expression arrow function syntax causes the result of push() to be implicitly returned; this should be wrapped in braces to use block form.
Suggested fix
- tailIndices.forEach(index => collapsed.push(crumbs[index]));
+ tailIndices.forEach(index => {
+ collapsed.push(crumbs[index]);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tailIndices.forEach(index => collapsed.push(crumbs[index])); | |
| tailIndices.forEach(index => { | |
| collapsed.push(crumbs[index]); | |
| }); |
🧰 Tools
🪛 Biome (2.4.4)
[error] 1067-1067: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/S3PathControl.tsx` at line 1067, The forEach callback on
tailIndices is using a single-expression arrow which implicitly returns the
result of collapsed.push(crumbs[index]) and triggers the lint rule; change the
callback to a block-bodied arrow (e.g., tailIndices.forEach(index => {
collapsed.push(crumbs[index]); });) so the push call is executed without an
implicit return. Ensure you update the invocation where tailIndices, collapsed,
and crumbs are used in S3PathControl to use the braced callback form.
| try { | ||
| await navigator.clipboard?.writeText(value); | ||
| } catch { | ||
| // Ignore clipboard errors. | ||
| } | ||
|
|
||
| setInfoMessage(copyMessage); | ||
| onCopy?.(value); |
There was a problem hiding this comment.
Only show copy success when clipboard write actually succeeds.
At Lines [1081]-[1088], "Path copied." is shown even when clipboard write fails, which can mislead users.
Suggested fix
- try {
- await navigator.clipboard?.writeText(value);
- } catch {
- // Ignore clipboard errors.
- }
-
- setInfoMessage(copyMessage);
+ let copied = false;
+ try {
+ await navigator.clipboard?.writeText(value);
+ copied = true;
+ } catch {
+ // Ignore clipboard errors.
+ }
+
+ if (copied) {
+ setInfoMessage(copyMessage);
+ }
onCopy?.(value);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await navigator.clipboard?.writeText(value); | |
| } catch { | |
| // Ignore clipboard errors. | |
| } | |
| setInfoMessage(copyMessage); | |
| onCopy?.(value); | |
| let copied = false; | |
| try { | |
| await navigator.clipboard?.writeText(value); | |
| copied = true; | |
| } catch { | |
| // Ignore clipboard errors. | |
| } | |
| if (copied) { | |
| setInfoMessage(copyMessage); | |
| } | |
| onCopy?.(value); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/S3PathControl.tsx` around lines 1081 - 1088, The current
copy logic calls setInfoMessage(copyMessage) and onCopy(value) regardless of
whether navigator.clipboard?.writeText(value) succeeds; change the flow in
S3PathControl (the block using navigator.clipboard?.writeText, setInfoMessage
and onCopy) so that setInfoMessage(copyMessage) and onCopy?.(value) are executed
only inside the try after await navigator.clipboard?.writeText(value) completes
successfully, and keep the catch block for silently ignoring or optionally
setting an error message; this ensures "Path copied." is only shown when the
clipboard write actually succeeds.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
web/src/ui/shared/s3-profile-selector/S3ProfileSelector.tsx (1)
80-80:flattenedProfilesmemo is redundant.Line [80] memoizes
filteredProfileswithout transformation. UsefilteredProfilesdirectly to reduce noise.♻️ Simplification
-const flattenedProfiles = useMemo(() => filteredProfiles, [filteredProfiles]); +const flattenedProfiles = filteredProfiles;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ui/shared/s3-profile-selector/S3ProfileSelector.tsx` at line 80, The variable flattenedProfiles is a redundant memo: remove the useMemo line that defines flattenedProfiles and replace any uses of flattenedProfiles with filteredProfiles directly (i.e., delete the const flattenedProfiles = useMemo(() => filteredProfiles, [filteredProfiles]) and update references in S3ProfileSelector to read from filteredProfiles), keeping useMemo only where an actual transformation is needed.web/src/ui/shared/s3-profile-selector/S3ProfileSelector.stories.tsx (1)
39-91: Reduce duplicated story args to avoid drift.
onSelect,onCreateRequested, and several common args are repeated across stories (Line [39] to Line [91]). Extract abaseArgsobject and spread it in each story to keep updates safer.♻️ Proposed refactor
+const baseArgs = { + profiles, + onSelect: () => {}, + onCreateRequested: () => {} +}; export const ClosedState: Story = { args: { - profiles, + ...baseArgs, activeProfileId: "managed-1", profileCreationPolicy: "allowed", - defaultOpen: false, - onSelect: () => {}, - onCreateRequested: () => {} + defaultOpen: false } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ui/shared/s3-profile-selector/S3ProfileSelector.stories.tsx` around lines 39 - 91, Extract common story args into a shared baseArgs object and spread it into each story to eliminate duplication: create baseArgs containing profiles, onSelect, onCreateRequested, and any other repeated defaults (e.g., defaultOpen if appropriate), then update ClosedState, OpenState, EmptyState, CreationDisabled, and CreationRestricted to use args: { ...baseArgs, <overrides like activeProfileId, profileCreationPolicy, defaultOpen, profiles: [] for EmptyState, etc.> } so each story only specifies its unique overrides.web/src/ui/shared/bookmarks/EntryPointsList.stories.tsx (1)
15-31: Consider explicit type annotation for mock data.Adding explicit types to the mock data arrays improves type safety and catches mismatches early:
💡 Optional type annotation
+import type { BucketEntry, Bookmark } from "./types"; + -const buckets = [ +const buckets: BucketEntry[] = [ { label: "analytics-data", // ... } ]; -const pinned = [ +const pinned: (Bookmark & { subLabel?: string })[] = [ { id: "pin-exports", // ... } ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ui/shared/bookmarks/EntryPointsList.stories.tsx` around lines 15 - 31, The mock "buckets" array lacks an explicit type; add a type annotation to the "buckets" constant (or reuse the existing domain/interface if one exists, e.g., Bucket, EntryPoint, or similar) so TypeScript can validate properties and allowed "type" values; update the declaration for the buckets symbol in EntryPointsList.stories.tsx to use a strongly-typed array (or union for the "type" field) that matches the component's expected prop types.web/src/ui/shared/bookmarks/BookmarkNameModal.tsx (1)
55-124: Consider adding displayName for memoized components.The
BodyandButtonscomponents are wrapped inmemobut lackdisplayName. This can make debugging harder in React DevTools. The outerBookmarkNameModalalso lacks it.💡 Add displayName for debugging
const Body = memo((props: BodyProps) => { // ... }); +Body.displayName = "Body"; const Buttons = memo((props: ButtonsProps) => { // ... }); +Buttons.displayName = "Buttons";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ui/shared/bookmarks/BookmarkNameModal.tsx` around lines 55 - 124, The memoized components lack displayName which hurts React DevTools debugging; add displayName properties after their memo definitions — e.g. set Body.displayName = "BookmarkNameModal.Body", Buttons.displayName = "BookmarkNameModal.Buttons", and BookmarkNameModal.displayName = "BookmarkNameModal" (use the exact component identifiers Body, Buttons, BookmarkNameModal) so that React shows meaningful names for these memo-wrapped components.web/src/ui/shared/bookmarks/BookmarkChip.stories.tsx (1)
41-48: Story name doesn't match behavior.
WithOptionalUnpinprovides anonUnpinhandler, which is the same as other stories. Based onBookmarkChip.tsx, the unpin button becomes disabled whenonUnpinis undefined. Consider either renaming this story or testing the truly "optional" case by omittingonUnpin.💡 Option to test disabled unpin state
export const WithOptionalUnpin: Story = { args: { - label: "To unpin", + label: "No unpin handler", path: "s3://example-bucket", - onNavigate: () => undefined, - onUnpin: () => undefined + onNavigate: () => undefined + // onUnpin intentionally omitted to show disabled state } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ui/shared/bookmarks/BookmarkChip.stories.tsx` around lines 41 - 48, The story named WithOptionalUnpin is misleading because it currently supplies onUnpin (so the unpin button is enabled); either rename the story to reflect that it includes an unpin handler (e.g., WithUnpin) or modify its args to omit the onUnpin prop to test the disabled-unpin state; locate the story export WithOptionalUnpin in BookmarkChip.stories.tsx and adjust the args to remove onUnpin (or rename the export) so behavior and name match the component logic in BookmarkChip.tsx.web/src/ui/shared/bookmarks/EntryPointsList.tsx (1)
45-54: Add a deterministic tie-breaker for buckets with the same priority.Lines 45-54 return
0for same-type buckets, which leaves order dependent on incoming array sequence. A secondarylabelsort gives stable UX.♻️ Suggested fix
const orderedBuckets = [...buckets].sort((a, b) => { const aPriority = bucketPriority.get(a.type) ?? bucketOrder.length; const bPriority = bucketPriority.get(b.type) ?? bucketOrder.length; @@ if (aPriority !== bPriority) { return aPriority - bPriority; } - return 0; + return a.label.localeCompare(b.label, undefined, { sensitivity: "base" }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ui/shared/bookmarks/EntryPointsList.tsx` around lines 45 - 54, The current sort in orderedBuckets uses bucketPriority and returns 0 on ties, leaving order unstable; update the comparator in the sort call (the function passed to [...buckets].sort) to, after comparing aPriority and bPriority, tie-break by comparing a.label and b.label (e.g., using localeCompare) so buckets with equal priority are deterministically ordered; ensure you handle missing/undefined labels (fall back to empty string) when invoking localeCompare.web/src/ui/shared/bookmarks/BookmarkRowItem.tsx (1)
8-80: Extract the Material Symbols loader into a shared utility.Lines 8-80 duplicate logic that also exists in
web/src/ui/shared/bookmarks/BookmarkChip.tsx(Line 14-38). Centralizing it will avoid drift and inconsistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/ui/shared/bookmarks/BookmarkRowItem.tsx` around lines 8 - 80, The Material Symbols loader (ensureMaterialSymbols and materialSymbolsHref with the fixed linkId behavior) is duplicated; extract it into a single shared utility (export a function ensureMaterialSymbols that uses the same linkId "material-symbols-outlined-keep" and materialSymbolsHref constant and preserves the existing DOM checks/update/append logic), then remove the local ensureMaterialSymbols and materialSymbolsHref from BookmarkRowItem.tsx and BookmarkChip.tsx and replace them with an import of the shared ensureMaterialSymbols utility so both files call the centralized implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/ui/shared/bookmarks/BookmarkChip.tsx`:
- Around line 135-181: The chip and pinButton style objects (chip, pinButton in
BookmarkChip.tsx) lack :focus-visible rules for keyboard focus; add
:focus-visible entries for both to show a visible, accessible focus indicator
(e.g., outline or boxShadow) that contrasts with theme and respects
isUnpinDisabled for pinButton (no focus style when disabled), and ensure the
focus style does not rely on :hover selectors so keyboard users see it. Use the
same pattern used for &:hover (e.g., "&:focus-visible": { ... }) and reference
theme/isDarkModeEnabled and accent to compute colors consistently with existing
styles.
- Around line 53-59: The Unpin button in BookmarkChip uses aria-disabled without
the native disabled attribute, so add disabled={!onUnpin} to the <button> (using
the existing onUnpin prop and path in the onClick) and remove the aria-disabled
prop to avoid a semantic mismatch; ensure the click handler remains optional
(onUnpin?.(path)) so the button is truly inert when disabled and keep the
className={classes.pinButton} unchanged.
In `@web/src/ui/shared/bookmarks/BookmarkPinButton.tsx`:
- Around line 8-15: The buttonStyle uses cursor: "pointer" unconditionally which
prevents showing a disabled cursor; update the BookmarkPinButton component to
set the cursor based on the disabled state (e.g., compute style or merge a
disabled style so cursor becomes "not-allowed" when disabled) — change the
styling logic around buttonStyle in BookmarkPinButton to use disabled ? {
...buttonStyle, cursor: "not-allowed" } : buttonStyle (or define a
disabledButtonStyle) and ensure the disabled prop/attribute on the button is
honored.
In `@web/src/ui/shared/bookmarks/BookmarkRowItem.tsx`:
- Around line 256-283: The unpinButton style lacks a keyboard-visible focus
state; add a &:focus-visible rule inside unpinButton to provide a clear focus
indicator (e.g., outline or boxShadow and visible icon swap similar to &:hover)
so keyboard users can see focus, and ensure you toggle
.pinIconDefault/.pinIconHover visibility in that &:focus-visible block (refer to
unpinButton, .pinIconDefault, .pinIconHover and the existing &:hover logic to
mirror behavior for keyboard focus).
In `@web/src/ui/shared/bookmarks/BucketTypeChip.tsx`:
- Around line 35-42: BucketTypeChip renders the material icon span using class
"material-symbols-outlined" but doesn't guard loading the font; add a useEffect
in the BucketTypeChip component that calls ensureMaterialSymbols() on mount
(similar to BookmarkChip/BookmarkRowItem) so icons in standalone contexts (e.g.,
Storybook) load correctly; locate the BucketTypeChip component and add the
import/use of ensureMaterialSymbols() and call it inside useEffect when
showIcon/tagIcons are used to render the span with classes.icon.
In `@web/src/ui/shared/bookmarks/EntryPointsList.tsx`:
- Line 83: In EntryPointsList, the active prop currently compares item.path ===
currentPath which fails for equivalent paths like "s3://bucket" vs
"s3://bucket/"; add a small normalization step (e.g., a normalizePath helper
used in the component) that trims trailing slashes and handles undefined/empty
values, then change the active check to compare normalizePath(item.path) ===
normalizePath(currentPath); update any other comparisons in this component that
rely on raw path strings to use the same helper.
In `@web/src/ui/shared/bookmarks/getDefaultBookmarkLabelFromPath.ts`:
- Around line 18-22: In getDefaultBookmarkLabelFromPath, the if (rest.length ===
0) branch is dead code because earlier branches already handle parts length 0
and 1, so after const [, ...rest] the rest array will always contain at least
one element; remove the unreachable if (rest.length === 0) { return parts[0]; }
block and any related unreachable returns, keeping the const [, ...rest]
destructuring (or replacing it with direct indexing if clearer) and ensure the
remaining logic that uses rest/parts continues to produce the correct label.
In `@web/src/ui/shared/bookmarks/PinnedChipsBar.tsx`:
- Line 41: The useEffect in PinnedChipsBar.tsx currently depends on
bookmarks.length which misses changes when items are reordered or replaced;
update the dependency to a stable representation of the bookmarks array (for
example use the full bookmarks reference or a derived key like bookmarks.map(b
=> b.id).join(',') or a useMemo'd ids array) so the effect that recalculates
fade indicators runs when bookmarks change order or identity; locate the effect
hook that ends with "}, [bookmarks.length]);" and replace the dependency with
the chosen stable key (or bookmarks) and, if deriving ids, compute them with
useMemo to avoid unnecessary recalculations.
In `@web/src/ui/shared/s3-profile-selector/S3ProfileSelector.tsx`:
- Around line 17-40: The component injects a remote Google Fonts stylesheet at
runtime via materialSymbolsHref and ensureMaterialSymbols (creating an element
with linkId and appending to document.head), which violates CSP and causes
third-party requests per render; remove the runtime injection from
S3ProfileSelector and instead load the Material Symbols stylesheet at the app
level (e.g., global HTML/head or a top-level layout) or replace usage with
local/static icon assets, updating any references in S3ProfileSelector to rely
on the globally loaded stylesheet or the local icon components.
- Around line 229-233: S3ProfileSelector currently sets role="listbox" on the
dropdown wrapper (classes.dropdown) while focusing a child with
role="presentation" and rendering selectable rows as buttons, which breaks ARIA
listbox/option semantics; fix by choosing one of two consistent approaches: (A)
if you want a true listbox, remove role="presentation", give each selectable row
the role="option", manage focus/arrow key navigation in onListKeyDown to move
focus to those options and set aria-selected appropriately, or (B) drop
role="listbox" from classes.dropdown, remove any role="presentation" usage, and
rely on the native button semantics for each selectable row (keep them as
<button> elements, ensure keyboard handlers are attached to the buttons and that
onListKeyDown is adjusted or removed). Ensure the change is applied to
S3ProfileSelector, update onListKeyDown accordingly, and keep
aria-selected/state and focus behavior consistent with the chosen pattern.
- Around line 235-240: The search input in S3ProfileSelector uses
placeholder-only accessibility; add an explicit accessible name by adding an
aria-label (or a visible label element tied to the input) to the input element
that currently uses classes.searchInput and value={searchValue} with
onChange={event => setSearchValue(event.target.value)} so screen readers get a
stable control name; ensure the label text is descriptive like "Search profiles"
and, if using a label element, associate it via htmlFor matching the input's id.
---
Nitpick comments:
In `@web/src/ui/shared/bookmarks/BookmarkChip.stories.tsx`:
- Around line 41-48: The story named WithOptionalUnpin is misleading because it
currently supplies onUnpin (so the unpin button is enabled); either rename the
story to reflect that it includes an unpin handler (e.g., WithUnpin) or modify
its args to omit the onUnpin prop to test the disabled-unpin state; locate the
story export WithOptionalUnpin in BookmarkChip.stories.tsx and adjust the args
to remove onUnpin (or rename the export) so behavior and name match the
component logic in BookmarkChip.tsx.
In `@web/src/ui/shared/bookmarks/BookmarkNameModal.tsx`:
- Around line 55-124: The memoized components lack displayName which hurts React
DevTools debugging; add displayName properties after their memo definitions —
e.g. set Body.displayName = "BookmarkNameModal.Body", Buttons.displayName =
"BookmarkNameModal.Buttons", and BookmarkNameModal.displayName =
"BookmarkNameModal" (use the exact component identifiers Body, Buttons,
BookmarkNameModal) so that React shows meaningful names for these memo-wrapped
components.
In `@web/src/ui/shared/bookmarks/BookmarkRowItem.tsx`:
- Around line 8-80: The Material Symbols loader (ensureMaterialSymbols and
materialSymbolsHref with the fixed linkId behavior) is duplicated; extract it
into a single shared utility (export a function ensureMaterialSymbols that uses
the same linkId "material-symbols-outlined-keep" and materialSymbolsHref
constant and preserves the existing DOM checks/update/append logic), then remove
the local ensureMaterialSymbols and materialSymbolsHref from BookmarkRowItem.tsx
and BookmarkChip.tsx and replace them with an import of the shared
ensureMaterialSymbols utility so both files call the centralized implementation.
In `@web/src/ui/shared/bookmarks/EntryPointsList.stories.tsx`:
- Around line 15-31: The mock "buckets" array lacks an explicit type; add a type
annotation to the "buckets" constant (or reuse the existing domain/interface if
one exists, e.g., Bucket, EntryPoint, or similar) so TypeScript can validate
properties and allowed "type" values; update the declaration for the buckets
symbol in EntryPointsList.stories.tsx to use a strongly-typed array (or union
for the "type" field) that matches the component's expected prop types.
In `@web/src/ui/shared/bookmarks/EntryPointsList.tsx`:
- Around line 45-54: The current sort in orderedBuckets uses bucketPriority and
returns 0 on ties, leaving order unstable; update the comparator in the sort
call (the function passed to [...buckets].sort) to, after comparing aPriority
and bPriority, tie-break by comparing a.label and b.label (e.g., using
localeCompare) so buckets with equal priority are deterministically ordered;
ensure you handle missing/undefined labels (fall back to empty string) when
invoking localeCompare.
In `@web/src/ui/shared/s3-profile-selector/S3ProfileSelector.stories.tsx`:
- Around line 39-91: Extract common story args into a shared baseArgs object and
spread it into each story to eliminate duplication: create baseArgs containing
profiles, onSelect, onCreateRequested, and any other repeated defaults (e.g.,
defaultOpen if appropriate), then update ClosedState, OpenState, EmptyState,
CreationDisabled, and CreationRestricted to use args: { ...baseArgs, <overrides
like activeProfileId, profileCreationPolicy, defaultOpen, profiles: [] for
EmptyState, etc.> } so each story only specifies its unique overrides.
In `@web/src/ui/shared/s3-profile-selector/S3ProfileSelector.tsx`:
- Line 80: The variable flattenedProfiles is a redundant memo: remove the
useMemo line that defines flattenedProfiles and replace any uses of
flattenedProfiles with filteredProfiles directly (i.e., delete the const
flattenedProfiles = useMemo(() => filteredProfiles, [filteredProfiles]) and
update references in S3ProfileSelector to read from filteredProfiles), keeping
useMemo only where an actual transformation is needed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
web/src/ui/shared/S3ExplorerNavigationBar.tsxweb/src/ui/shared/bookmarks/BookmarkChip.stories.tsxweb/src/ui/shared/bookmarks/BookmarkChip.tsxweb/src/ui/shared/bookmarks/BookmarkNameModal.tsxweb/src/ui/shared/bookmarks/BookmarkPinButton.tsxweb/src/ui/shared/bookmarks/BookmarkRowItem.tsxweb/src/ui/shared/bookmarks/BookmarksOverview.stories.tsxweb/src/ui/shared/bookmarks/BucketTypeChip.stories.tsxweb/src/ui/shared/bookmarks/BucketTypeChip.tsxweb/src/ui/shared/bookmarks/EntryPointsList.stories.tsxweb/src/ui/shared/bookmarks/EntryPointsList.tsxweb/src/ui/shared/bookmarks/PinnedChipsBar.tsxweb/src/ui/shared/bookmarks/getDefaultBookmarkLabelFromPath.tsweb/src/ui/shared/bookmarks/types.tsweb/src/ui/shared/s3-profile-selector/S3ProfileSelector.stories.tsxweb/src/ui/shared/s3-profile-selector/S3ProfileSelector.tsxweb/src/ui/shared/s3-profile-selector/index.tsweb/src/ui/shared/s3-profile-selector/types.ts
✅ Files skipped from review due to trivial changes (1)
- web/src/ui/shared/s3-profile-selector/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/ui/shared/S3ExplorerNavigationBar.tsx
| <button | ||
| type="button" | ||
| aria-label="Unpin bookmark" | ||
| aria-disabled={!onUnpin} | ||
| onClick={() => onUnpin?.(path)} | ||
| className={classes.pinButton} | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n web/src/ui/shared/bookmarks/BookmarkChip.tsx | sed -n '50,65p'Repository: InseeFrLab/onyxia
Length of output: 857
🏁 Script executed:
cat -n web/src/ui/shared/bookmarks/BookmarkChip.tsx | head -30Repository: InseeFrLab/onyxia
Length of output: 1130
🏁 Script executed:
cat -n web/src/ui/shared/bookmarks/BookmarkChip.tsx | sed -n '80,200p'Repository: InseeFrLab/onyxia
Length of output: 4933
🏁 Script executed:
cat -n web/src/ui/shared/bookmarks/BookmarkChip.tsx | sed -n '35,75p'Repository: InseeFrLab/onyxia
Length of output: 1814
Use native disabled attribute for the Unpin button when onUnpin is absent.
On line 56, aria-disabled is set without the native disabled attribute, creating a semantic mismatch. The button remains interactive (optional chaining in the click handler at line 57 simply does nothing if onUnpin is undefined), while aria-disabled only communicates the disabled state to screen readers. Using disabled={!onUnpin} properly prevents interaction and removes the button from the tab order.
🛠️ Suggested fix
<button
type="button"
aria-label="Unpin bookmark"
- aria-disabled={!onUnpin}
+ disabled={!onUnpin}
onClick={() => onUnpin?.(path)}
className={classes.pinButton}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| type="button" | |
| aria-label="Unpin bookmark" | |
| aria-disabled={!onUnpin} | |
| onClick={() => onUnpin?.(path)} | |
| className={classes.pinButton} | |
| > | |
| <button | |
| type="button" | |
| aria-label="Unpin bookmark" | |
| disabled={!onUnpin} | |
| onClick={() => onUnpin?.(path)} | |
| className={classes.pinButton} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/bookmarks/BookmarkChip.tsx` around lines 53 - 59, The Unpin
button in BookmarkChip uses aria-disabled without the native disabled attribute,
so add disabled={!onUnpin} to the <button> (using the existing onUnpin prop and
path in the onClick) and remove the aria-disabled prop to avoid a semantic
mismatch; ensure the click handler remains optional (onUnpin?.(path)) so the
button is truly inert when disabled and keep the className={classes.pinButton}
unchanged.
| chip: { | ||
| display: "inline-flex", | ||
| alignItems: "center", | ||
| border: "none", | ||
| background: "transparent", | ||
| color: "inherit", | ||
| padding: 0, | ||
| font: "inherit", | ||
| cursor: "pointer", | ||
| maxWidth: "100%", | ||
| minWidth: 0, | ||
| overflow: "hidden" | ||
| }, | ||
| labelText: { | ||
| display: "block", | ||
| whiteSpace: "nowrap", | ||
| overflow: "hidden", | ||
| textOverflow: "ellipsis" | ||
| }, | ||
| pinButton: { | ||
| border: "none", | ||
| background: "transparent", | ||
| color: "inherit", | ||
| cursor: isUnpinDisabled ? "default" : "pointer", | ||
| width: 20, | ||
| height: 20, | ||
| borderRadius: 6, | ||
| display: "inline-flex", | ||
| alignItems: "center", | ||
| justifyContent: "center", | ||
| padding: 0, | ||
| opacity: isUnpinDisabled ? 0.4 : 1, | ||
| "&:hover": { | ||
| backgroundColor: isUnpinDisabled | ||
| ? "transparent" | ||
| : alpha(accent, theme.isDarkModeEnabled ? 0.3 : 0.2) | ||
| }, | ||
| "& .pinIconHover": { | ||
| display: "none" | ||
| }, | ||
| "&:hover .pinIconDefault": { | ||
| display: "none" | ||
| }, | ||
| "&:hover .pinIconHover": { | ||
| display: "inline-flex" | ||
| } | ||
| }, |
There was a problem hiding this comment.
Add :focus-visible styles on both interactive buttons.
Lines 135-147 (chip) and 154-181 (pinButton) define clickable controls without focus-visible styling, which weakens keyboard accessibility.
♿ Suggested fix
chip: {
display: "inline-flex",
alignItems: "center",
border: "none",
background: "transparent",
color: "inherit",
padding: 0,
font: "inherit",
cursor: "pointer",
maxWidth: "100%",
minWidth: 0,
- overflow: "hidden"
+ overflow: "hidden",
+ "&:focus-visible": {
+ outline: `2px solid ${theme.colors.useCases.typography.textFocus}`,
+ outlineOffset: 2,
+ borderRadius: 6
+ }
},
@@
pinButton: {
border: "none",
background: "transparent",
color: "inherit",
cursor: isUnpinDisabled ? "default" : "pointer",
width: 20,
height: 20,
borderRadius: 6,
display: "inline-flex",
alignItems: "center",
justifyContent: "center",
padding: 0,
opacity: isUnpinDisabled ? 0.4 : 1,
+ "&:focus-visible": {
+ outline: `2px solid ${theme.colors.useCases.typography.textFocus}`,
+ outlineOffset: 2
+ },
"&:hover": {
backgroundColor: isUnpinDisabled
? "transparent"
: alpha(accent, theme.isDarkModeEnabled ? 0.3 : 0.2)
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/bookmarks/BookmarkChip.tsx` around lines 135 - 181, The
chip and pinButton style objects (chip, pinButton in BookmarkChip.tsx) lack
:focus-visible rules for keyboard focus; add :focus-visible entries for both to
show a visible, accessible focus indicator (e.g., outline or boxShadow) that
contrasts with theme and respects isUnpinDisabled for pinButton (no focus style
when disabled), and ensure the focus style does not rely on :hover selectors so
keyboard users see it. Use the same pattern used for &:hover (e.g.,
"&:focus-visible": { ... }) and reference theme/isDarkModeEnabled and accent to
compute colors consistently with existing styles.
| const buttonStyle: CSSProperties = { | ||
| border: "1px solid #cbd5e1", | ||
| background: "#ffffff", | ||
| borderRadius: 6, | ||
| padding: "6px 10px", | ||
| fontSize: 13, | ||
| cursor: "pointer" | ||
| }; |
There was a problem hiding this comment.
Missing disabled state styling.
The cursor: "pointer" style is applied unconditionally. When the button is disabled, the cursor should change (e.g., to "not-allowed") for better UX feedback.
💅 Proposed fix for disabled cursor
-const buttonStyle: CSSProperties = {
- border: "1px solid `#cbd5e1`",
- background: "#ffffff",
- borderRadius: 6,
- padding: "6px 10px",
- fontSize: 13,
- cursor: "pointer"
-};
-
-export function BookmarkPinButton(props: BookmarkPinButtonProps) {
- const { onClick, disabled } = props;
-
- return (
- <button type="button" onClick={onClick} disabled={disabled} style={buttonStyle}>
+const getButtonStyle = (disabled?: boolean): CSSProperties => ({
+ border: "1px solid `#cbd5e1`",
+ background: "#ffffff",
+ borderRadius: 6,
+ padding: "6px 10px",
+ fontSize: 13,
+ cursor: disabled ? "not-allowed" : "pointer",
+ opacity: disabled ? 0.6 : 1
+});
+
+export function BookmarkPinButton(props: BookmarkPinButtonProps) {
+ const { onClick, disabled } = props;
+
+ return (
+ <button type="button" onClick={onClick} disabled={disabled} style={getButtonStyle(disabled)}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/bookmarks/BookmarkPinButton.tsx` around lines 8 - 15, The
buttonStyle uses cursor: "pointer" unconditionally which prevents showing a
disabled cursor; update the BookmarkPinButton component to set the cursor based
on the disabled state (e.g., compute style or merge a disabled style so cursor
becomes "not-allowed" when disabled) — change the styling logic around
buttonStyle in BookmarkPinButton to use disabled ? { ...buttonStyle, cursor:
"not-allowed" } : buttonStyle (or define a disabledButtonStyle) and ensure the
disabled prop/attribute on the button is honored.
| unpinButton: { | ||
| position: "absolute", | ||
| top: 24, | ||
| right: 24, | ||
| border: "none", | ||
| background: "transparent", | ||
| cursor: "pointer", | ||
| padding: 0, | ||
| display: "inline-flex", | ||
| alignItems: "center", | ||
| justifyContent: "center", | ||
| width: 24, | ||
| height: 24, | ||
| borderRadius: 6, | ||
| "&:hover": { | ||
| color: theme.colors.useCases.buttons.actionActive, | ||
| backgroundColor: alpha(accent, theme.isDarkModeEnabled ? 0.3 : 0.2) | ||
| }, | ||
| "& .pinIconHover": { | ||
| display: "none" | ||
| }, | ||
| "&:hover .pinIconDefault": { | ||
| display: "none" | ||
| }, | ||
| "&:hover .pinIconHover": { | ||
| display: "inline-flex" | ||
| } | ||
| }, |
There was a problem hiding this comment.
Add keyboard-visible focus styling for the Unpin control.
Line 256 defines an interactive button, but there is no :focus-visible state. Keyboard users won’t get a clear focus indicator on Line 138’s unpin action.
♿ Suggested fix
unpinButton: {
position: "absolute",
top: 24,
right: 24,
border: "none",
background: "transparent",
cursor: "pointer",
padding: 0,
display: "inline-flex",
alignItems: "center",
justifyContent: "center",
width: 24,
height: 24,
borderRadius: 6,
+ "&:focus-visible": {
+ outline: `2px solid ${theme.colors.useCases.typography.textFocus}`,
+ outlineOffset: 2
+ },
"&:hover": {
color: theme.colors.useCases.buttons.actionActive,
backgroundColor: alpha(accent, theme.isDarkModeEnabled ? 0.3 : 0.2)
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/bookmarks/BookmarkRowItem.tsx` around lines 256 - 283, The
unpinButton style lacks a keyboard-visible focus state; add a &:focus-visible
rule inside unpinButton to provide a clear focus indicator (e.g., outline or
boxShadow and visible icon swap similar to &:hover) so keyboard users can see
focus, and ensure you toggle .pinIconDefault/.pinIconHover visibility in that
&:focus-visible block (refer to unpinButton, .pinIconDefault, .pinIconHover and
the existing &:hover logic to mirror behavior for keyboard focus).
| showIcon ? ( | ||
| <span | ||
| className={`material-symbols-outlined ${classes.icon}`} | ||
| aria-hidden="true" | ||
| > | ||
| {tagIcons[type]} | ||
| </span> | ||
| ) : undefined |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the BucketTypeChip component
cat -n web/src/ui/shared/bookmarks/BucketTypeChip.tsxRepository: InseeFrLab/onyxia
Length of output: 4408
🏁 Script executed:
# Check if BookmarkChip exists and examine it
find . -name "BookmarkChip.tsx" -o -name "BookmarkChip.ts" | head -5Repository: InseeFrLab/onyxia
Length of output: 107
🏁 Script executed:
# Check if BookmarkRowItem exists and examine it
find . -name "BookmarkRowItem.tsx" -o -name "BookmarkRowItem.ts" | head -5Repository: InseeFrLab/onyxia
Length of output: 110
🏁 Script executed:
# Search for Material Symbols stylesheet imports across the codebase
rg "material-symbols" --type ts --type tsx -iRepository: InseeFrLab/onyxia
Length of output: 88
🏁 Script executed:
# Search for Material Symbols imports with correct syntax
rg "material-symbols" -iRepository: InseeFrLab/onyxia
Length of output: 1707
🏁 Script executed:
# Examine BookmarkChip component
cat -n web/src/ui/shared/bookmarks/BookmarkChip.tsxRepository: InseeFrLab/onyxia
Length of output: 8119
🏁 Script executed:
# Examine BookmarkRowItem component
cat -n web/src/ui/shared/bookmarks/BookmarkRowItem.tsxRepository: InseeFrLab/onyxia
Length of output: 12007
🏁 Script executed:
# Check for global Material Symbols stylesheet loading in entry points
find . -name "index.tsx" -o -name "index.ts" -o -name "App.tsx" -o -name "main.tsx" | head -10Repository: InseeFrLab/onyxia
Length of output: 458
🏁 Script executed:
# Search for Material Symbols href or stylesheet in root-level files
rg "Material\+Symbols" -i --max-count=20Repository: InseeFrLab/onyxia
Length of output: 603
🏁 Script executed:
# Check if there's a global CSS or HTML file that loads Material Symbols
find . -name "*.html" -o -name "index.css" -o -name "global.css" | head -5Repository: InseeFrLab/onyxia
Length of output: 77
🏁 Script executed:
# Let's check the public/index.html or entry point
find . -path "*/public/index.html" -o -path "*/src/index.html"Repository: InseeFrLab/onyxia
Length of output: 43
🏁 Script executed:
# Examine the main index.html file
cat -n web/index.htmlRepository: InseeFrLab/onyxia
Length of output: 10608
🏁 Script executed:
# Check if BucketTypeChip is used in contexts where Material Symbols might be globally available
rg "BucketTypeChip" -A 3 -B 3Repository: InseeFrLab/onyxia
Length of output: 4954
🏁 Script executed:
# Check the storybook configuration for Material Symbols
cat -n web/.storybook/main.tsxRepository: InseeFrLab/onyxia
Length of output: 748
🏁 Script executed:
# Check if there are any other usages of BucketTypeChip besides BookmarkRowItem
rg "BucketTypeChip" --type tsx --type ts -B 2 -A 2 | grep -v "stories\|test"Repository: InseeFrLab/onyxia
Length of output: 88
🏁 Script executed:
# Examine the storybook preview configuration for any global stylesheet loading
find web/.storybook -name "preview.*" -o -name "preview-head.html"Repository: InseeFrLab/onyxia
Length of output: 87
🏁 Script executed:
# Check if Material Symbols is globally loaded in any preview or setup file
find web/.storybook -type f | head -10Repository: InseeFrLab/onyxia
Length of output: 172
🏁 Script executed:
# Examine the storybook preview file
cat -n web/.storybook/preview.tsxRepository: InseeFrLab/onyxia
Length of output: 2322
🏁 Script executed:
# Check if BucketTypeChip is used elsewhere besides BookmarkRowItem and Storybook
rg "BucketTypeChip" -iRepository: InseeFrLab/onyxia
Length of output: 1433
🏁 Script executed:
# Search for any global CSS or Material Symbols references in storybook static or public directories
find web/public -name "*.css" -o -name "*.html" 2>/dev/null | head -5Repository: InseeFrLab/onyxia
Length of output: 43
Add defensive Material Symbols font loading to BucketTypeChip.
This component uses material-symbols-outlined but lacks the ensureMaterialSymbols() guard present in BookmarkChip and BookmarkRowItem. While production usage is protected (only rendered within BookmarkRowItem), standalone contexts—particularly Storybook—will render icon glyphs as text if the stylesheet hasn't been loaded. Add ensureMaterialSymbols() via useEffect for consistency and safety.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/bookmarks/BucketTypeChip.tsx` around lines 35 - 42,
BucketTypeChip renders the material icon span using class
"material-symbols-outlined" but doesn't guard loading the font; add a useEffect
in the BucketTypeChip component that calls ensureMaterialSymbols() on mount
(similar to BookmarkChip/BookmarkRowItem) so icons in standalone contexts (e.g.,
Storybook) load correctly; locate the BucketTypeChip component and add the
import/use of ensureMaterialSymbols() and call it inside useEffect when
showIcon/tagIcons are used to render the span with classes.icon.
| const [, ...rest] = parts; | ||
|
|
||
| if (rest.length === 0) { | ||
| return parts[0]; | ||
| } |
There was a problem hiding this comment.
Unreachable code block.
The condition rest.length === 0 at line 20 can never be true. At this point, parts.length >= 2 (since earlier branches handle length 0 and 1), so after destructuring [, ...rest], rest will always have at least one element.
🧹 Proposed fix to remove dead code
const [, ...rest] = parts;
-
- if (rest.length === 0) {
- return parts[0];
- }
-
const tail = rest.slice(-3);
return tail.join("-");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [, ...rest] = parts; | |
| if (rest.length === 0) { | |
| return parts[0]; | |
| } | |
| const [, ...rest] = parts; | |
| const tail = rest.slice(-3); | |
| return tail.join("-"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/bookmarks/getDefaultBookmarkLabelFromPath.ts` around lines
18 - 22, In getDefaultBookmarkLabelFromPath, the if (rest.length === 0) branch
is dead code because earlier branches already handle parts length 0 and 1, so
after const [, ...rest] the rest array will always contain at least one element;
remove the unreachable if (rest.length === 0) { return parts[0]; } block and any
related unreachable returns, keeping the const [, ...rest] destructuring (or
replacing it with direct indexing if clearer) and ensure the remaining logic
that uses rest/parts continues to produce the correct label.
| element.removeEventListener("scroll", updateFade); | ||
| window.removeEventListener("resize", updateFade); | ||
| }; | ||
| }, [bookmarks.length]); |
There was a problem hiding this comment.
Effect dependency may miss bookmark changes.
Using bookmarks.length as the dependency means the fade indicators won't recalculate when bookmarks are reordered or replaced without changing the total count. Consider using a stable key derived from bookmarks (e.g., mapping ids) or the full array reference.
🔧 Proposed fix
- }, [bookmarks.length]);
+ }, [bookmarks]);If concerned about performance with frequent bookmark reference changes, derive a stable key:
+ const bookmarksKey = bookmarks.map(b => b.id).join(",");
+
useEffect(() => {
// ...
- }, [bookmarks.length]);
+ }, [bookmarksKey]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }, [bookmarks.length]); | |
| }, [bookmarks]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/bookmarks/PinnedChipsBar.tsx` at line 41, The useEffect in
PinnedChipsBar.tsx currently depends on bookmarks.length which misses changes
when items are reordered or replaced; update the dependency to a stable
representation of the bookmarks array (for example use the full bookmarks
reference or a derived key like bookmarks.map(b => b.id).join(',') or a
useMemo'd ids array) so the effect that recalculates fade indicators runs when
bookmarks change order or identity; locate the effect hook that ends with "},
[bookmarks.length]);" and replace the dependency with the chosen stable key (or
bookmarks) and, if deriving ids, compute them with useMemo to avoid unnecessary
recalculations.
| const materialSymbolsHref = | ||
| "https://fonts.googleapis.com/css2?family=Material+Symbols+Outlined:opsz,wght,FILL,GRAD@20..48,400,0,0&icon_names=family_group,person,warning,error"; | ||
|
|
||
| function ensureMaterialSymbols() { | ||
| if (typeof document === "undefined") { | ||
| return; | ||
| } | ||
|
|
||
| const linkId = "material-symbols-outlined-s3-profile"; | ||
| const existing = document.getElementById(linkId) as HTMLLinkElement | null; | ||
|
|
||
| if (existing) { | ||
| if (existing.href !== materialSymbolsHref) { | ||
| existing.href = materialSymbolsHref; | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| const link = document.createElement("link"); | ||
| link.id = linkId; | ||
| link.rel = "stylesheet"; | ||
| link.href = materialSymbolsHref; | ||
| document.head.appendChild(link); | ||
| } |
There was a problem hiding this comment.
Avoid runtime injection of third-party Google Fonts from the component.
Line [17] to Line [40] and Line [61] to Line [63] add a remote stylesheet at runtime. This can fail under strict CSP and introduces third-party requests from UI render paths. Prefer app-level asset loading (or local icons) rather than per-component injection.
Also applies to: 61-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/s3-profile-selector/S3ProfileSelector.tsx` around lines 17
- 40, The component injects a remote Google Fonts stylesheet at runtime via
materialSymbolsHref and ensureMaterialSymbols (creating an element with linkId
and appending to document.head), which violates CSP and causes third-party
requests per render; remove the runtime injection from S3ProfileSelector and
instead load the Material Symbols stylesheet at the app level (e.g., global
HTML/head or a top-level layout) or replace usage with local/static icon assets,
updating any references in S3ProfileSelector to rely on the globally loaded
stylesheet or the local icon components.
| <div | ||
| className={classes.dropdown} | ||
| role="listbox" | ||
| onKeyDown={onListKeyDown} | ||
| > |
There was a problem hiding this comment.
Listbox semantics are inconsistent with the rendered/focused elements.
Line [231] sets role="listbox" on the dropdown wrapper, but focus is moved to a child with role="presentation" (Line [247]) and selectable rows are buttons (Line [316]). This breaks expected listbox/option semantics for screen readers. Align roles/focus model (true listbox+option pattern) or drop listbox roles and use native button semantics end-to-end.
Also applies to: 243-248, 316-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/s3-profile-selector/S3ProfileSelector.tsx` around lines 229
- 233, S3ProfileSelector currently sets role="listbox" on the dropdown wrapper
(classes.dropdown) while focusing a child with role="presentation" and rendering
selectable rows as buttons, which breaks ARIA listbox/option semantics; fix by
choosing one of two consistent approaches: (A) if you want a true listbox,
remove role="presentation", give each selectable row the role="option", manage
focus/arrow key navigation in onListKeyDown to move focus to those options and
set aria-selected appropriately, or (B) drop role="listbox" from
classes.dropdown, remove any role="presentation" usage, and rely on the native
button semantics for each selectable row (keep them as <button> elements, ensure
keyboard handlers are attached to the buttons and that onListKeyDown is adjusted
or removed). Ensure the change is applied to S3ProfileSelector, update
onListKeyDown accordingly, and keep aria-selected/state and focus behavior
consistent with the chosen pattern.
| <input | ||
| className={classes.searchInput} | ||
| placeholder="Search profiles" | ||
| value={searchValue} | ||
| onChange={event => setSearchValue(event.target.value)} | ||
| /> |
There was a problem hiding this comment.
Add an explicit accessible name to the search input.
The input at Line [235] only has a placeholder. Add aria-label (or visible label) so assistive technologies expose a stable control name.
✅ Minimal fix
<input
className={classes.searchInput}
+ aria-label="Search S3 profiles"
placeholder="Search profiles"
value={searchValue}
onChange={event => setSearchValue(event.target.value)}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <input | |
| className={classes.searchInput} | |
| placeholder="Search profiles" | |
| value={searchValue} | |
| onChange={event => setSearchValue(event.target.value)} | |
| /> | |
| <input | |
| className={classes.searchInput} | |
| aria-label="Search S3 profiles" | |
| placeholder="Search profiles" | |
| value={searchValue} | |
| onChange={event => setSearchValue(event.target.value)} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/ui/shared/s3-profile-selector/S3ProfileSelector.tsx` around lines 235
- 240, The search input in S3ProfileSelector uses placeholder-only
accessibility; add an explicit accessible name by adding an aria-label (or a
visible label element tied to the input) to the input element that currently
uses classes.searchInput and value={searchValue} with onChange={event =>
setSearchValue(event.target.value)} so screen readers get a stable control name;
ensure the label text is descriptive like "Search profiles" and, if using a
label element, associate it via htmlFor matching the input's id.
marchufschmitt
force-pushed
the
s3-ui-codex
branch
from
9ac0289 to
bbb9707
Compare
|
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores