WIP: Migrating from bespoke 's3Configs' that where assotiated to a wo… #1038
Conversation
…rkingDir to standard s3Profile
WalkthroughRefactors S3 configuration into a new unified S3Profile model, replaces admin-bookmark resolution with per-config templated bookmarks, removes working directory concepts, adjusts selectors/state/thunks accordingly, and drops STS bucket creation. Introduces profile aggregation and ID helpers. Updates exports and connection test flow to a placeholder error. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI
participant Thunk as s3ConfigManagement/thunks.initialize
participant Region as DeploymentRegion S3Configs
participant OIDC as OIDC Client
participant Resolver as resolveTemplatedBookmark
participant Store as Redux Store
UI->>Thunk: initialize()
Thunk->>Region: get s3Configs + bookmarks
loop for each region s3Config
Thunk->>OIDC: getDecodedIdToken (per-config)
OIDC-->>Thunk: decodedIdToken
Thunk->>Resolver: resolveTemplatedBookmark({ bookmark_region, getDecodedIdToken })
Resolver-->>Thunk: S3Profile.Bookmark[]
end
Thunk->>Store: actions.initialized({ resolvedTemplatedBookmarks })
Store-->>UI: state ready
sequenceDiagram
autonumber
actor UI
participant Selector as selectors.s3Profiles
participant Agg as aggregateS3ProfilesFromVaultAndRegionIntoAnUnifiedSet
participant Vault as ProjectConfigs.s3
participant Region as DeploymentRegion S3Configs
participant Tests as ConnectionTestsState
UI->>Selector: select()
Selector->>Agg: aggregate({ fromVault: Vault, fromRegion: Region+bookmarks, connectionTestsState: Tests })
Agg-->>Selector: S3Profile[]
Selector-->>UI: S3Profile[]
sequenceDiagram
autonumber
actor UI
participant Thunk as s3ConfigConnectionTest/testS3Connection
participant Store as Redux Store
UI->>Thunk: testS3Connection({ paramsOfCreateS3Client })
Thunk->>Store: actions.testStarted({ paramsOfCreateS3Client })
Note over Thunk: TODO path not implemented
Thunk-->>Thunk: throw "Not implemented yet"
Thunk->>Store: actions.testCompleted({ paramsOfCreateS3Client, result: isSuccess=false, errorMessage })
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/src/core/usecases/s3ConfigManagement/thunks.ts (2)
218-236: Update hash to use resolvedTemplatedBookmarks (old selector remains).Still referencing resolvedAdminBookmarks; with templated-bookmarks flow this should switch to resolvedTemplatedBookmarks to keep token cache invalidation correct.
- const resolvedAdminBookmarks = - privateSelectors.resolvedAdminBookmarks(getState()); - - const KEY = "onyxia:s3:resolvedAdminBookmarks-hash"; - - const hash = fnv1aHashToHex( - JSON.stringify(resolvedAdminBookmarks) - ); + const resolvedTemplatedBookmarks = + privateSelectors.resolvedTemplatedBookmarks(getState()); + + const KEY = "onyxia:s3:resolvedTemplatedBookmarks-hash"; + + const hash = fnv1aHashToHex( + JSON.stringify(resolvedTemplatedBookmarks) + );Please also verify privateSelectors exposes resolvedTemplatedBookmarks accordingly.
3-3: Migrate deprecatedS3ConfigtoS3Profile
- Replace
withimport type { S3Config } from "./decoupledLogic/getS3Configs";import type { S3Profile } from "./decoupledLogic/s3Profiles";- Change the return signature
toPromise<undefined | { s3Client: S3Client; s3Config: S3Config }>Promise<undefined | { s3Client: S3Client; s3Config: S3Profile }>- Update any remaining
ProjectConfigs.S3Configreferences (e.g. in thecreateS3Configthunk) toProjectConfigs.S3Profileweb/src/core/usecases/s3ConfigConnectionTest/thunks.ts (1)
29-34: Do not ship the connection test always throwingThe happy-path was replaced with a console log and
throw new Error("TODO: Not implemented yet"), so every S3 connection test now deterministically fails. This breaks a user-facing feature and prevents validating credentials. Please restore the previous verification logic (or provide a working replacement) before merging.
🧹 Nitpick comments (9)
web/src/core/adapters/s3Client/index.ts (1)
52-57: Remove the empty comment block.This comment block contains no content and should be removed to maintain code cleanliness.
Apply this diff:
-/* - - - - -*/web/src/core/usecases/s3ConfigManagement/decoupledLogic/updateDefaultS3ProfilesAfterPotentialDeletion.ts (1)
16-65: Fallback selection logic is sound.Choosing the first region-origin profile when the current default is gone is reasonable.
Consider renaming local variables from s3ConfigId_* to s3ProfileId_* for clarity within this profile-centric helper.
web/src/core/usecases/s3ConfigManagement/thunks.ts (1)
318-367: Memoize token fetch and prefetch OIDC params to avoid N× OIDC inits.Currently, each bookmark triggers a fresh createOidc/getTokens per config, and oidc params are refetched per config. Cache per-config tokens and fetch oidcParams once.
- const resolvedTemplatedBookmarks = await Promise.all( + const { oidcParams } = + await onyxiaApi.getAvailableRegionsAndOidcParams(); + assert(oidcParams !== undefined); + + const resolvedTemplatedBookmarks = await Promise.all( deploymentRegion.s3Configs.map(async (s3Config, s3ConfigIndex) => { const { bookmarks, sts: { oidcParams: oidcParams_partial } } = s3Config; - const getDecodedIdToken = async () => { - const { createOidc, mergeOidcParams } = await import( - "core/adapters/oidc" - ); - - const { oidcParams } = - await onyxiaApi.getAvailableRegionsAndOidcParams(); - - assert(oidcParams !== undefined); - - const oidc = await createOidc({ - ...mergeOidcParams({ - oidcParams, - oidcParams_partial - }), - autoLogin: true, - transformBeforeRedirectForKeycloakTheme: - paramsOfBootstrapCore.transformBeforeRedirectForKeycloakTheme, - getCurrentLang: paramsOfBootstrapCore.getCurrentLang, - enableDebugLogs: paramsOfBootstrapCore.enableOidcDebugLogs - }); - - const { decodedIdToken } = await oidc.getTokens(); - - return decodedIdToken; - }; + const getDecodedIdToken = (() => { + let pr: Promise<Record<string, unknown>> | undefined; + return async () => { + if (pr !== undefined) return pr; + pr = (async () => { + const { createOidc, mergeOidcParams } = await import( + "core/adapters/oidc" + ); + const oidc = await createOidc({ + ...mergeOidcParams({ + oidcParams, + oidcParams_partial + }), + autoLogin: true, + transformBeforeRedirectForKeycloakTheme: + paramsOfBootstrapCore.transformBeforeRedirectForKeycloakTheme, + getCurrentLang: paramsOfBootstrapCore.getCurrentLang, + enableDebugLogs: paramsOfBootstrapCore.enableOidcDebugLogs + }); + const { decodedIdToken } = await oidc.getTokens(); + return decodedIdToken; + })(); + return pr; + }; + })(); return { correspondingS3ConfigIndexInRegion: s3ConfigIndex, bookmarks: ( await Promise.all( bookmarks.map(bookmark => resolveTemplatedBookmark({ bookmark_region: bookmark, getDecodedIdToken }) ) ) ).flat() }; }) ); dispatch(actions.initialized({ resolvedTemplatedBookmarks }));Also applies to: 368-369
web/src/core/usecases/s3ConfigManagement/decoupledLogic/resolveTemplatedBookmark.ts (1)
83-119: Make the filter a proper type guard
filter(x => x !== undefined)doesn’t narrow the union, so the expression still has type(S3Profile.Bookmark | undefined)[], which contradicts the declaredPromise<S3Profile.Bookmark[]>return type understrictNullChecks. Please turn the predicate into a type guard so the compiler understands the array is purified.- .filter(x => x !== undefined); + .filter( + (bookmark): bookmark is S3Profile.Bookmark => bookmark !== undefined + );web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profiles.ts (5)
1-1: Use type-only import to avoid unnecessary runtime importThis module uses
ProjectConfigsonly for typing. Import it as type to reduce bundle and avoid side-effects.-import * as projectManagement from "core/usecases/projectManagement"; +import type * as projectManagement from "core/usecases/projectManagement";
38-44: Prefer optional overT | undefinedforBookmark.tagsIdiomatic TS favors optional properties for presence/absence. Keeps shapes simpler and narrows easier.
- tags: LocalizedString[] | undefined; + tags?: LocalizedString[];
60-89: Simplify connection-test status logic; remove labeled block and usesomeSame behavior, cleaner flow; avoids labeled block and double scans are unchanged.
- if ( - connectionTestsState.ongoing.find(e => - same(e.paramsOfCreateS3Client, paramsOfCreateS3Client) - ) !== undefined - ) { + if ( + connectionTestsState.ongoing.some(e => + same(e.paramsOfCreateS3Client, paramsOfCreateS3Client) + ) + ) { return { status: "test ongoing" }; } - has_result: { - const { result } = - connectionTestsState.results.find(e => - same(e.paramsOfCreateS3Client, paramsOfCreateS3Client) - ) ?? {}; - - if (result === undefined) { - break has_result; - } - - return result.isSuccess - ? { status: "test succeeded" } - : { status: "test failed", errorMessage: result.errorMessage }; - } - - return { status: "not tested" }; + const match = connectionTestsState.results.find(e => + same(e.paramsOfCreateS3Client, paramsOfCreateS3Client) + ); + if (!match) { + return { status: "not tested" }; + } + return match.result.isSuccess + ? { status: "test succeeded" } + : { status: "test failed", errorMessage: match.result.errorMessage };
98-105: Profiles retain credentials; ensure they aren’t logged or leaked
paramsOfCreateS3Client.credentialsembeds secrets in the aggregated object. Avoid loggings3Profilesand scrub in telemetry.Please confirm no logs/analytics serialize these objects. If needed, I can suggest a redaction helper for debug logs.
155-183: Defaults assignment: clarify fallback intent; minor readability
- Fallback picks the first region profile if the ID is missing. This may set both flags on the same region. Confirm this is intended.
- Using
for...ofimproves readability overforEachwith earlyreturns.- Rename the inner variable from
s3Configtos3Profilefor clarity.- ( - [ - ["defaultXOnyxia", fromVault.s3ConfigId_defaultXOnyxia], - ["explorer", fromVault.s3ConfigId_explorer] - ] as const - ).forEach(([prop, s3ProfileId]) => { - if (s3ProfileId === undefined) { - return; - } - - const s3Profile = - s3Profiles.find(({ id }) => id === s3ProfileId) ?? - s3Profiles.find(s3Config => s3Config.origin === "region"); - - if (s3Profile === undefined) { - return; - } - - switch (prop) { - case "defaultXOnyxia": - s3Profile.isXOnyxiaDefault = true; - return; - case "explorer": - s3Profile.isExplorerConfig = true; - return; - } - assert<Equals<typeof prop, never>>(false); - }); + for (const [prop, s3ProfileId] of [ + ["defaultXOnyxia", fromVault.s3ConfigId_defaultXOnyxia], + ["explorer", fromVault.s3ConfigId_explorer] + ] as const) { + if (s3ProfileId === undefined) continue; + const s3Profile = + s3Profiles.find(({ id }) => id === s3ProfileId) ?? + s3Profiles.find(p => p.origin === "region"); + if (!s3Profile) continue; + if (prop === "defaultXOnyxia") { + s3Profile.isXOnyxiaDefault = true; + continue; + } + s3Profile.isExplorerConfig = true; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
web/src/core/adapters/s3Client/index.ts(1 hunks)web/src/core/adapters/s3Client/s3Client.ts(0 hunks)web/src/core/ports/OnyxiaApi/DeploymentRegion.ts(2 hunks)web/src/core/usecases/projectManagement/decoupledLogic/ProjectConfigs.ts(0 hunks)web/src/core/usecases/s3ConfigConnectionTest/state.ts(3 hunks)web/src/core/usecases/s3ConfigConnectionTest/thunks.ts(2 hunks)web/src/core/usecases/s3ConfigManagement/decoupledLogic/getS3Configs.ts(0 hunks)web/src/core/usecases/s3ConfigManagement/decoupledLogic/getWorkingDirectoryBucket.ts(0 hunks)web/src/core/usecases/s3ConfigManagement/decoupledLogic/getWorkingDirectoryPath.ts(0 hunks)web/src/core/usecases/s3ConfigManagement/decoupledLogic/projectS3ConfigId.ts(0 hunks)web/src/core/usecases/s3ConfigManagement/decoupledLogic/resolveS3AdminBookmarks.ts(0 hunks)web/src/core/usecases/s3ConfigManagement/decoupledLogic/resolveTemplatedBookmark.ts(1 hunks)web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profile_fromVault_id.ts(1 hunks)web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profiles.ts(1 hunks)web/src/core/usecases/s3ConfigManagement/decoupledLogic/updateDefaultS3ConfigsAfterPotentialDeletion.ts(0 hunks)web/src/core/usecases/s3ConfigManagement/decoupledLogic/updateDefaultS3ProfilesAfterPotentialDeletion.ts(1 hunks)web/src/core/usecases/s3ConfigManagement/index.ts(1 hunks)web/src/core/usecases/s3ConfigManagement/selectors.ts(2 hunks)web/src/core/usecases/s3ConfigManagement/state.ts(2 hunks)web/src/core/usecases/s3ConfigManagement/thunks.ts(3 hunks)
💤 Files with no reviewable changes (8)
- web/src/core/usecases/projectManagement/decoupledLogic/ProjectConfigs.ts
- web/src/core/usecases/s3ConfigManagement/decoupledLogic/getS3Configs.ts
- web/src/core/usecases/s3ConfigManagement/decoupledLogic/projectS3ConfigId.ts
- web/src/core/usecases/s3ConfigManagement/decoupledLogic/updateDefaultS3ConfigsAfterPotentialDeletion.ts
- web/src/core/adapters/s3Client/s3Client.ts
- web/src/core/usecases/s3ConfigManagement/decoupledLogic/getWorkingDirectoryPath.ts
- web/src/core/usecases/s3ConfigManagement/decoupledLogic/getWorkingDirectoryBucket.ts
- web/src/core/usecases/s3ConfigManagement/decoupledLogic/resolveS3AdminBookmarks.ts
🧰 Additional context used
🧬 Code graph analysis (8)
web/src/core/usecases/s3ConfigManagement/decoupledLogic/updateDefaultS3ProfilesAfterPotentialDeletion.ts (3)
web/src/core/ports/OnyxiaApi/DeploymentRegion.ts (2)
DeploymentRegion(4-104)S3Config(107-123)web/src/core/usecases/projectManagement/decoupledLogic/ProjectConfigs.ts (2)
S3Config(22-35)ProjectConfigs(9-19)web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profiles.ts (1)
aggregateS3ProfilesFromVaultAndRegionIntoAnUnifiedSet(47-185)
web/src/core/usecases/s3ConfigConnectionTest/thunks.ts (1)
web/src/core/adapters/s3Client/s3Client.ts (2)
ParamsOfCreateS3Client(22-24)NoSts(33-42)
web/src/core/usecases/s3ConfigManagement/state.ts (1)
web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profiles.ts (2)
S3Profile(11-11)Bookmark(38-44)
web/src/core/usecases/s3ConfigManagement/decoupledLogic/resolveTemplatedBookmark.ts (2)
web/src/core/ports/OnyxiaApi/DeploymentRegion.ts (2)
DeploymentRegion(4-104)Bookmark(126-143)web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profiles.ts (2)
Bookmark(38-44)S3Profile(11-11)
web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profiles.ts (5)
web/src/core/adapters/s3Client/s3Client.ts (4)
Common(27-31)ParamsOfCreateS3Client(22-24)Sts(44-55)NoSts(33-42)web/src/core/ports/OnyxiaApi/DeploymentRegion.ts (3)
Bookmark(126-143)DeploymentRegion(4-104)S3Config(107-123)web/src/core/usecases/projectManagement/decoupledLogic/ProjectConfigs.ts (2)
ProjectConfigs(9-19)S3Config(22-35)web/src/core/usecases/s3ConfigConnectionTest/state.ts (2)
ConfigTestResult(15-25)OngoingConfigTest(11-13)web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profile_fromVault_id.ts (1)
s3Profile_fromVault_getId(7-11)
web/src/core/usecases/s3ConfigManagement/selectors.ts (1)
web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profiles.ts (2)
S3Profile(11-11)aggregateS3ProfilesFromVaultAndRegionIntoAnUnifiedSet(47-185)
web/src/core/ports/OnyxiaApi/DeploymentRegion.ts (4)
web/src/core/ports/OnyxiaApi/index.ts (2)
DeploymentRegion(4-4)LocalizedString(6-6)web/src/core/usecases/projectManagement/decoupledLogic/ProjectConfigs.ts (1)
S3Config(22-35)web/src/core/usecases/projectManagement/decoupledLogic/projectConfigsMigration/v0ToV1.ts (1)
S3Config(78-92)web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profiles.ts (1)
Bookmark(38-44)
web/src/core/usecases/s3ConfigManagement/thunks.ts (2)
web/src/core/adapters/oidc/oidc.ts (2)
createOidc(13-105)mergeOidcParams(107-125)web/src/core/usecases/s3ConfigManagement/decoupledLogic/resolveTemplatedBookmark.ts (1)
resolveTemplatedBookmark(8-120)
🪛 ast-grep (0.39.6)
web/src/core/usecases/s3ConfigManagement/decoupledLogic/resolveTemplatedBookmark.ts
[warning] 78-78: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(includedClaimPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 80-80: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(excludedClaimPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (3)
web/src/core/usecases/s3ConfigManagement/thunks.ts (2)
15-15: New helper import — OK.Import of resolveTemplatedBookmark is correct.
34-38: Connection test payload trim — OK.Dropping workingDirectoryPath aligns with the new state shape.
web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profiles.ts (1)
13-45: LGTM on the unified S3Profile model and namespacingClear union with well-scoped variants, strong typing of STS vs NoSTS, and bookmark modeling looks solid.
| const x = { | ||
| workingDirectory: { | ||
| bucketMode: "multi", | ||
| bucketNamePrefix: "", | ||
| bucketNamePrefixGroup: "projet-" | ||
| }, | ||
| bookmarkedDirectories: [ | ||
| { | ||
| fullPath: "donnees-insee/diffusion/", | ||
| title: { | ||
| fr: "Données de diffusion", | ||
| en: "Dissemination Data" | ||
| }, | ||
| description: { | ||
| fr: "Bucket public destiné à la diffusion de données", | ||
| en: "Public bucket intended for data dissemination" | ||
| } | ||
| } | ||
| ] | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Remove or export the unused constant.
The constant x is defined but not exported, making it inaccessible to other modules. The non-descriptive name and lack of usage suggest this is development scaffolding that should be removed.
Run the following script to verify this constant is not referenced elsewhere:
🏁 Script executed:
#!/bin/bash
# Description: Search for any references to the constant 'x' from this module.
# Search for imports or usage of this constant
rg -n --type=ts --type=tsx 'from.*s3Client.*x|s3Client.*\.x' -g '!web/src/core/adapters/s3Client/index.ts'Length of output: 27
Run the following script to verify there are no references to the constant x:
🏁 Script executed:
#!/bin/bash
# Search for named imports of `x` from any module
rg -nP -- 'import\s+\{[^}]*\bx\b[^}]*\}' -g '*.ts' -g '*.tsx'
# Search for any property access on a variable named `x`
rg -nP -- '\bx\.' -g '*.ts' -g '*.tsx'Length of output: 353
Run the following script to verify there are no references to the constant x:
🏁 Script executed:
#!/bin/bash
# Search for named imports of `x` in TS/TSX files
rg -n --glob '*.ts' --glob '*.tsx' 'import\s+{[^}]*\bx\b[^}]*}' .
# Search for any direct property access on `x`
rg -n --glob '*.ts' --glob '*.tsx' '\bx\.' .Length of output: 256
Remove unused constant x
The constant x is defined but never exported or referenced; remove it or rename and export it if it’s required.
🤖 Prompt for AI Agents
In web/src/core/adapters/s3Client/index.ts around lines 3 to 22, there is an
unused constant named `x`; remove the dead constant declaration to eliminate
unused code (or if intended to be used, rename it to a descriptive identifier
and export or reference it where needed). Ensure imports/exports and linting
pass after removing or exporting/renaming the constant.
| const s3 = { | ||
| bookmarkedDirectories: [ | ||
| { | ||
| fullPath: "$1/", | ||
| title: "Personal", | ||
| description: "Personal storage", | ||
| claimName: "preferred_username" | ||
| }, | ||
| { | ||
| fullPath: "projet-$1/", | ||
| title: "Group $1", | ||
| description: "Shared storage for project $1", | ||
| claimName: "groups", | ||
| excludedClaimPattern: "^USER_ONYXIA$" | ||
| }, | ||
| { | ||
| fullPath: "donnees-insee/diffusion/", | ||
| title: { | ||
| fr: "Données de diffusion", | ||
| en: "Dissemination Data" | ||
| }, | ||
| description: { | ||
| fr: "Bucket public destiné à la diffusion de données", | ||
| en: "Public bucket intended for data dissemination" | ||
| } | ||
| } | ||
| ] | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Remove or properly utilize the unused constant.
The constant s3 is defined but not exported, rendering it inaccessible. While the structure appears to be a valid S3 profile configuration template (with claim-based path templating), it serves no purpose in its current form.
Consider one of the following actions:
- Remove if this is leftover scaffolding
- Export if it's meant to be a reference example
- Move to documentation if it's illustrative
Run the following script to verify this constant is not referenced elsewhere:
🏁 Script executed:
#!/bin/bash
# Description: Search for any references to the constant 's3' from this module.
# Search for imports or usage of this constant
rg -n --type=ts --type=tsx 'from.*s3Client.*\bs3\b|s3Client\.\bs3\b' -g '!web/src/core/adapters/s3Client/index.ts'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Description: Search for any references to the constant 's3' from this module.
# Search for import statements or usage of s3Client across TS and TSX files
rg -n "s3Client" -g "*.ts" -g "*.tsx" -g "!web/src/core/adapters/s3Client/index.ts"Length of output: 6225
Remove or export the unused s3 constant.
The s3 object is declared but never exported or referenced within the codebase, resulting in dead code. Either delete it if it’s leftover scaffolding, or export it (or relocate it to documentation) if it’s intended for reuse.
🤖 Prompt for AI Agents
In web/src/core/adapters/s3Client/index.ts around lines 23 to 50 there is an
unused constant s3 that creates bookmarkedDirectories but is neither exported
nor referenced; remove the dead constant if it’s leftover scaffolding, or
alternatively export it (e.g., export const s3 = ...) or move it into
documentation/config where intended for reuse so the symbol is either used or
intentionally exposed.
| const { paramsOfCreateS3Client } = payload; | ||
|
|
||
| if ( | ||
| state.ongoingConfigTests.find(e => | ||
| same(e, { paramsOfCreateS3Client, workingDirectoryPath }) | ||
| same(e, { paramsOfCreateS3Client }) | ||
| ) !== undefined | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| state.ongoingConfigTests.push({ | ||
| paramsOfCreateS3Client, | ||
| workingDirectoryPath | ||
| }); | ||
| state.ongoingConfigTests.push({ paramsOfCreateS3Client }); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Keying by paramsOfCreateS3Client only — LGTM.
Deep-compare usage is correct; dedupe and replace semantics look right.
Run to spot stale references to workingDirectoryPath:
Also applies to: 64-85
🏁 Script executed:
#!/usr/bin/env bash
rg -n --type=ts --type=tsx -C2 '\bworkingDirectoryPath\b'Length of output: 27
🏁 Script executed:
#!/usr/bin/env bash
rg -n '\bworkingDirectoryPath\b' -C2 .Length of output: 22376
Remove stale workingDirectoryPath references across S3 config usecases
Residual workingDirectoryPath usages in web/src/core/usecases/s3ConfigCreation/thunks.ts, state.ts and selectors.ts must be updated or removed to align with the new dedupe logic keyed solely by paramsOfCreateS3Client.
🤖 Prompt for AI Agents
In web/src/core/usecases/s3ConfigConnectionTest/state.ts around lines 44-55
there are stale references to workingDirectoryPath in the dedupe logic; remove
any usage of workingDirectoryPath and ensure deduplication and state entries are
keyed only by paramsOfCreateS3Client (i.e., replace same(e, {
paramsOfCreateS3Client, workingDirectoryPath }) checks with comparisons that
only compare paramsOfCreateS3Client, update the object pushed into
state.ongoingConfigTests to contain only { paramsOfCreateS3Client }, and adjust
any related equality helpers or type definitions so the state shape and
comparisons no longer expect or reference workingDirectoryPath).
| export function s3Profile_fromVault_parseId(params: { s3ConfigId: string }): { | ||
| creationTime: number; | ||
| } { | ||
| const { s3ConfigId } = params; | ||
|
|
||
| const creationTimeStr = s3ConfigId.replace(prefix, ""); | ||
|
|
||
| const creationTime = parseInt(creationTimeStr); | ||
|
|
||
| assert(!isNaN(creationTime), "Not a valid s3 vault config id"); | ||
|
|
||
| return { creationTime }; | ||
| } |
There was a problem hiding this comment.
Enforce strict ID format when parsing.
Current parse accepts strings like "project-123abc" (parseInt truncates), and doesn’t require prefix at start.
Apply for stricter validation:
-export function s3Profile_fromVault_parseId(params: { s3ConfigId: string }): {
- creationTime: number;
-} {
- const { s3ConfigId } = params;
-
- const creationTimeStr = s3ConfigId.replace(prefix, "");
-
- const creationTime = parseInt(creationTimeStr);
-
- assert(!isNaN(creationTime), "Not a valid s3 vault config id");
-
- return { creationTime };
-}
+export function s3Profile_fromVault_parseId(params: { s3ConfigId: string }): {
+ creationTime: number;
+} {
+ const { s3ConfigId } = params;
+ const match = new RegExp(`^${prefix}(\\d+)$`).exec(s3ConfigId);
+ assert(match !== null, "Not a valid s3 vault config id");
+ const creationTime = Number(match[1]);
+ assert(
+ Number.isSafeInteger(creationTime),
+ "Invalid creationTime in s3 vault config id"
+ );
+ return { creationTime };
+}📝 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.
| export function s3Profile_fromVault_parseId(params: { s3ConfigId: string }): { | |
| creationTime: number; | |
| } { | |
| const { s3ConfigId } = params; | |
| const creationTimeStr = s3ConfigId.replace(prefix, ""); | |
| const creationTime = parseInt(creationTimeStr); | |
| assert(!isNaN(creationTime), "Not a valid s3 vault config id"); | |
| return { creationTime }; | |
| } | |
| export function s3Profile_fromVault_parseId(params: { s3ConfigId: string }): { | |
| creationTime: number; | |
| } { | |
| const { s3ConfigId } = params; | |
| const match = new RegExp(`^${prefix}(\\d+)$`).exec(s3ConfigId); | |
| assert(match !== null, "Not a valid s3 vault config id"); | |
| const creationTime = Number(match[1]); | |
| assert( | |
| Number.isSafeInteger(creationTime), | |
| "Invalid creationTime in s3 vault config id" | |
| ); | |
| return { creationTime }; | |
| } |
🤖 Prompt for AI Agents
In
web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profile_fromVault_id.ts
around lines 13 to 25, the parser currently strips the prefix anywhere and uses
parseInt which accepts trailing non-digits; change it to require the string to
start with the exact prefix (use startsWith), extract the suffix only if prefix
matches, validate the suffix matches /^\d+$/ (only digits), then convert with
Number or parseInt and assert it's a finite integer; update the assert message
to indicate either "Invalid s3 vault config id: missing prefix" or "Invalid s3
vault config id: creation time must be digits" as appropriate.
| return { | ||
| origin: "region", | ||
| id: `region-${fnv1aHashToHex( | ||
| JSON.stringify( | ||
| Object.fromEntries( | ||
| Object.entries(c).sort(([key1], [key2]) => | ||
| key1.localeCompare(key2) | ||
| ) | ||
| ) | ||
| ) | ||
| )}`, |
There was a problem hiding this comment.
Make region profile ID stable across nested object key orders
Only top-level key sorting is applied before hashing. Nested objects (e.g., sts) can reorder, producing different IDs for equivalent configs, causing flicker and broken defaults.
Use a stable stringify that sorts keys recursively:
- id: `region-${fnv1aHashToHex(
- JSON.stringify(
- Object.fromEntries(
- Object.entries(c).sort(([key1], [key2]) =>
- key1.localeCompare(key2)
- )
- )
- )
- )}`,
+ id: `region-${fnv1aHashToHex(stableStringify(c))}`,Add this helper near the top of the file:
function stableStringify(value: unknown): string {
if (value === null || typeof value !== "object") {
return JSON.stringify(value);
}
if (Array.isArray(value)) {
return "[" + value.map(stableStringify).join(",") + "]";
}
const obj = value as Record<string, unknown>;
const keys = Object.keys(obj).sort((a, b) => a.localeCompare(b));
const entries = keys.map(
k => JSON.stringify(k) + ":" + stableStringify(obj[k])
);
return "{" + entries.join(",") + "}";
}🤖 Prompt for AI Agents
In web/src/core/usecases/s3ConfigManagement/decoupledLogic/s3Profiles.ts around
lines 127-137, the region profile ID is generated by JSON.stringify of a
top-level key-sorted object, which fails to produce stable IDs when nested
object key orders differ; add a recursive stableStringify helper near the top of
the file (before it's used) that handles nulls, arrays, and sorts object keys
recursively, then replace the current
JSON.stringify(Object.fromEntries(Object.entries(c).sort(...))) call with
stableStringify(c) so the hash is computed from a deterministically ordered
representation of the entire object.
| export * from "./selectors"; | ||
| export * from "./thunks"; | ||
| export type { S3Config } from "./decoupledLogic/getS3Configs"; | ||
| export type { S3Profile } from "./decoupledLogic/s3Profiles"; |
There was a problem hiding this comment.
🧩 Analysis chain
Public API shift to S3Profile — verify downstream imports.
Looks good. Ensure no importers still pull S3Config from this module.
🏁 Script executed:
#!/usr/bin/env bash
# Find imports of S3Config coming from s3ConfigManagement barrel
rg -nP --type=ts --type=tsx -C2 'import\s+[^;]*\bS3Config\b[^;]*from\s+[\'"][^\'"]*usecases/s3ConfigManagement[\'"]'Length of output: 68
🏁 Script executed:
#!/usr/bin/env bash
# Find imports of S3Config coming from s3ConfigManagement barrel
rg -nP --type=ts --type=tsx -C2 "import\s+[^;]*\bS3Config\b[^;]*from\s+['\"][^'\"]*usecases/s3ConfigManagement['\"]"Length of output: 27
🏁 Script executed:
#!/usr/bin/env bash
# Find imports of S3Config coming from s3ConfigManagement barrel in .ts and .tsx files
rg -nPC2 "import\s+[^;]*\bS3Config\b[^;]*from\s+['\"][^'\"]*usecases/s3ConfigManagement['\"]" -g "*.ts" -g "*.tsx"Length of output: 1171
🏁 Script executed:
#!/usr/bin/env bash
# Find definition of S3Config type/interface in the codebase
rg -nP "export\s+(?:type|interface)\s+S3Config" -g "*.ts" -g "*.tsx"Length of output: 447
Update downstream S3Config imports
Replace imports of
import type { S3Config } from "core/usecases/s3ConfigManagement"in
- web/src/ui/pages/projectSettings/ProjectSettingsS3ConfigTab/S3ConfigCard.tsx:10
- web/src/ui/pages/projectSettings/ProjectSettingsS3ConfigTab/TestS3ConnectionButton.tsx:2
with
import type { S3Config } from "core/usecases/projectManagement/decoupledLogic/ProjectConfigs"🤖 Prompt for AI Agents
In web/src/core/usecases/s3ConfigManagement/index.ts around line 4, update
downstream imports: the exported type path has changed and callers should import
S3Config from the projectManagement decoupled logic. Replace usages in
web/src/ui/pages/projectSettings/ProjectSettingsS3ConfigTab/S3ConfigCard.tsx
(line ~10) and
web/src/ui/pages/projectSettings/ProjectSettingsS3ConfigTab/TestS3ConnectionButton.tsx
(line ~2) so they import type { S3Config } from
"core/usecases/projectManagement/decoupledLogic/ProjectConfigs" instead of
"core/usecases/s3ConfigManagement".
|
Superseeded by #1041 |
…rkingDir to standard s3Profile
Summary by CodeRabbit
New Features
Refactor
Chores