Channel restriction checker for AIA-Issue-011 compliance#79
Channel restriction checker for AIA-Issue-011 compliance#79Lina-AbuYousef wants to merge 16 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEnabled admin-gated sidebar items; renamed TAG-001 to Unifai in Slack setup; removed dateRange from Slack channel settings; added AWAITING_SYNC pipeline status and UI badge; added syncSlackChannel API and wired per-channel Sync actions through channel table and Slack integration UI; major backend admin-config and DI/container additions plus many removed legacy modules. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Client UI
participant API as frontend backendApi (`ui/client/src/api/slack.ts`)
participant Server as Backend
User->>UI: Click "Sync" on channel row
UI->>API: POST /slack/sync/{channelId} (syncSlackChannel)
API->>Server: POST forwarded to backend sync endpoint
Server-->>API: { source_id, status } (e.g., AWAITING_SYNC)
API-->>UI: returns { source_id, status }
UI->>UI: mark channel as embedding (optimistic), refetch channels & stats
UI->>API: GET channels/stats (refetch)
Server-->>API: updated channel list & stats
API-->>UI: deliver updated data
UI-->>User: show toast and update status badge (Awaiting Sync)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
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 |
🔍 SummaryThis PR implements channel restriction logic for Slack ingestion to comply with AIA-Issue-011. While the logic effectively filters channels based on privacy, external sharing, and naming conventions, the directory structure ( 🧩 File-by-file feedback[DOMAIN: BACKEND] rag/core/data_sources/types/slack/domain/channel/restriction_checker.py
[DOMAIN: BACKEND] rag/infrastructure/sources/slack/connector.py
[DOMAIN: BACKEND] rag/core/data_sources/types/slack/event/handlers/channel_created.py
🛠 Suggested Improvements1. Centralize Configuration # config/app_config.py
class AppConfig(SharedConfig):
# ... existing config ...
slack_restricted_prefixes: List[str] = ["erg-", "event-", "hr-"]
slack_restricted_keywords: List[str] = ["human-resources"]2. Refactor Checker to use Config # backend/services/slack/channel_checker.py (Recommended Path)
from config.app_config import AppConfig
class ChannelRestrictionChecker:
@staticmethod
def is_restricted(channel_name: str, is_private: bool, is_ext_shared: bool) -> bool:
config = AppConfig.get_instance()
# Use config.slack_restricted_prefixes, etc.
# ... logic ...3. Align with Project Structure
✅ What's Good
✍️ Suggested Commit Message
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/client/src/features/slack/SlackSetupInfo.tsx (1)
228-228:⚠️ Potential issue | 🟡 MinorTypo: "Past" should be "Paste".
This is user-facing instructional text.
Proposed fix
- Past this message in the channel after app access is granted: + Paste this message in the channel after app access is granted:
🧹 Nitpick comments (1)
ui/client/src/features/slack/SlackSetupInfo.tsx (1)
139-139: Stale comment: still references "TAG App".The comment should be updated to reflect the rename.
Proposed fix
- {/* TAG App Section */} + {/* Unifai App Section */}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
ui/client/src/features/slack/SlackAddSourcePage.tsx (1)
84-88:⚠️ Potential issue | 🟡 MinorInconsistent terminology in empty-selection toast.
The success and error toasts were updated to use "sync" language, but this validation toast still says "to proceed with embedding". Consider updating it to match the new terminology (e.g., "to proceed with syncing").
Proposed fix
toast({ title: "⚠️ No Channels Selected", - description: "Please select at least one channel to proceed with embedding.", + description: "Please select at least one channel to proceed.", variant: "destructive", });ui/client/src/features/slack/ChannelTable.tsx (2)
151-166:⚠️ Potential issue | 🟡 Minor"Awaiting Sync" missing from status filter options.
The
filterFnhandlesPIPELINE_STATUS.AWAITING_SYNC→"Awaiting Sync"(lines 127-129), but"Awaiting Sync"is not listed infilterOptions. Users won't be able to filter channels by this new status from the dropdown.Proposed fix
filterOptions: [ "Pending", + "Awaiting Sync", "In Progress", "Collecting",
284-292:⚠️ Potential issue | 🟡 MinorSync button lacks a disabled state for actively processing channels.
The delete button (line 312) correctly disables itself when
isEmbeddingActivelyProcessing(ch), but the sync button fires unconditionally. Triggering a sync on a channel that's already collecting/embedding could cause unexpected behavior or duplicate work.Proposed fix
<Button variant="ghost" size="sm" - className="p-2 text-muted-foreground hover:text-primary hover:bg-primary/10 rounded-lg transition-all duration-200" - onClick={() => onSyncClick(ch)} + className={`p-2 text-muted-foreground rounded-lg transition-all duration-200 ${ + isEmbeddingActivelyProcessing(ch) + ? "opacity-50 cursor-not-allowed" + : "hover:text-primary hover:bg-primary/10" + }`} + disabled={isEmbeddingActivelyProcessing(ch)} + onClick={() => !isEmbeddingActivelyProcessing(ch) && onSyncClick(ch)} + title={isEmbeddingActivelyProcessing(ch) ? "Channel is currently processing" : "Sync channel"} >Note:
isEmbeddingActivelyProcessingis imported at line 20 from../helpers, so it's already available in scope.ui/client/src/features/slack/AddSourceSection.tsx (2)
98-106:⚠️ Potential issue | 🟡 MinorPossible brief flicker between debounce completing and query starting.
setIsSearching(false)fires synchronously withsetDebouncedSearchTerm(...)on line 104-105. However,isLoadingfrom React Query won't becometrueuntil the next render cycle when the new query key triggers a fetch. This creates a single frame where neitherisSearchingnorisLoadingis true, potentially causing the spinner to briefly disappear and then reappear.A simple fix: keep
isSearchingtrue and let the query's own loading state take over by clearing it in an effect that watchesisLoading.
414-425:⚠️ Potential issue | 🟡 MinorCustom placeholder overlay is not accessible — no
<label>and emptyplaceholder.The overlaid
<div>with "Search Channels" is not associated with the input for screen readers. The actualplaceholderis an empty string and there's no<label htmlFor="channel-search">. Use a realplaceholderattribute or anaria-labelto ensure the input is accessible.♿ Quick fix — add aria-label
<Input id="channel-search" value={searchTerm} onChange={e => setSearchTerm(e.target.value)} - placeholder="" + placeholder="" + aria-label="Search Channels" className={`input-dark-theme pr-10 bg-input border-border ${searchTerm ? 'pl-3' : 'pl-28'}`} />
🤖 Fix all issues with AI agents
In `@ui/client/src/features/slack/AddSourceSection.tsx`:
- Line 510: Remove the unused constant notMember (const notMember =
c.is_app_member === false || c.is_app_member === null) from the row rendering in
AddSourceSection, or if you intend to show membership status, replace its
declaration with a direct use in the UI (e.g., pass (c.is_app_member === false
|| c.is_app_member === null) to the tooltip/indicator prop or render a visual
badge) so that the variable is either eliminated or actually used in the
rendering logic.
- Around line 568-570: The settings panel lookup should use the stored entity
map instead of only searching the paginated `channels` array; replace the
channel lookup in the block that checks `selectedChannels.length > 0 &&
lastSelectedChannel` (currently using `channels.find(c => getChannelUniqueId(c)
=== lastSelectedChannel)`) to first try
`selectedChannelEntities[lastSelectedChannel]` and fall back to
`channels.find(...)` only if the entity map is missing, mirroring the behavior
of `getSelectedChannels` (used around line 212); ensure you reference
`getChannelUniqueId`, `selectedChannelEntities`, `selectedChannels`, and
`lastSelectedChannel` so the panel doesn't disappear when pages are evicted or
refetched.
In `@ui/client/src/features/slack/SlackIntegration.tsx`:
- Around line 178-195: The channel status AWAITING_SYNC isn't classified so
channels added by handleSyncChannel (via trackEmbeddingStart) stay in
activeEmbedding and cause indefinite polling; fix by updating
getInactiveStatuses to include "AWAITING_SYNC" (so status checks stop polling
and scheduled runs handle it) or implement a timeout cleanup that removes stale
IDs from activeEmbedding after a configurable period; update references in
getActiveStatuses/getInactiveStatuses, ensure
handleSyncChannel/trackEmbeddingStart and the completion-detection useEffect
correctly respond to the new inactive status or the timeout removal.
🧹 Nitpick comments (8)
ui/client/src/api/slack.ts (2)
160-163: Inconsistent URL path prefix.
syncSlackChanneluses a leading slash (/slack/sync/...) while most other endpoints in this file omit it (e.g.,'slack/available.slack.channels.get','pipelines/embed'). Line 166 ("/slack/stats") also has a leading slash, so the codebase is already inconsistent. Depending on how the axiosbaseURLis configured, this may or may not matter—but it's worth standardizing.
137-138: Hardcoded frequency string couples API layer to presentation.
"Every 12 hours"is a display-oriented string baked into the data-fetching layer. If the sync interval changes, you'd need to update this string in multiple places (also hardcoded as"Every 12h"inChannelSettingsDrawer.tsxline 77). Consider defining this once as a constant.ui/client/src/features/slack/ChannelSettingsDrawer.tsx (1)
12-17:onSaveprop is accepted but never used.The drawer was simplified to read-only with only a "Close" button, yet
onSaveremains in the props interface and is destructured at line 24. This is dead code that could confuse future maintainers into thinking save functionality exists.♻️ Proposed cleanup
export interface ChannelSettingsDrawerProps { channel: EmbedChannel; isOpen: boolean; onClose: () => void; - onSave: (values: Record<string, string | boolean>) => void; } export function ChannelSettingsDrawer({ channel, isOpen, onClose, - onSave, }: ChannelSettingsDrawerProps) {This would also require removing the
onSaveprop from the caller inSlackIntegration.tsx(line 372).Also applies to: 20-25
ui/client/src/features/slack/PaginatedChannelTable.tsx (1)
30-30:pageSizeprop is accepted but never used;initialStatehardcodes8.The component accepts a
pageSizeprop (default6) but theDataTable'sinitialStateon line 117 hardcodespageSize: 8. Either wire the prop into the initial state or remove it from the interface to avoid confusion.♻️ Suggested fix
initialState={{ - pagination: { pageIndex: 0, pageSize: 8 }, + pagination: { pageIndex: 0, pageSize }, sorting: [], }}Also applies to: 117-117
ui/client/src/features/slack/AddSourceSection.tsx (4)
42-44: Unreachable scope branches —SCOPE.ALLandSCOPE.PRIVATEpaths are dead code.
scopeOptionsnow only exposesPUBLIC, yet the query function (lines 142-177),typesParam(lines 121-125), andcounts(lines 374-388) still branch onALLandPRIVATE. These paths are unreachable from the UI. Consider removing them to reduce cognitive load, or leave a comment explaining the intent to re-enable them later.
68-74:parseChannelUniqueIdis defined but never used.This function is neither called within this file nor exported. Remove it or add an export if it's intended for use elsewhere.
244-279: State updates nested inside a functional updater are a code smell.Calling
setChannelSettings,setSelectedChannelEntities, andsetLastSelectedChannelinside thesetSelectedChannelsupdater function is an anti-pattern — state updaters should be pure. While React 18's automatic batching makes this work in practice, it makes the logic harder to follow and test. Consider extracting this into a single handler that computes the new state first, then calls all setters sequentially outside the updater.
289-303:allNamesduplicatesselectableChannelIds— same filter + map logic.Both
selectableChannelIds(lines 293-296) andallNames(lines 298-303) computefilteredChannels.filter(!embedded).map(getChannelUniqueId). Remove one and reuse the other.♻️ Suggested fix — drop `allNames`, reuse `selectableChannelIds`
const selectableChannels = useMemo(() => filteredChannels.filter(c => !isChannelEmbedded(c)), [filteredChannels, isChannelEmbedded] ); const selectableChannelIds = useMemo(() => selectableChannels.map(c => getChannelUniqueId(c)), [selectableChannels] ); - - const allNames = useMemo(() => - filteredChannels - .filter(c => !isChannelEmbedded(c)) - .map(c => getChannelUniqueId(c)), - [filteredChannels, isChannelEmbedded] - ); - const allSelected = selectableChannelIds.length > 0 && selectableChannelIds.every(id => selectedChannels.includes(id)); + const allSelected = selectableChannelIds.length > 0 && selectableChannelIds.every(id => selectedChannels.includes(id));Then replace all
allNamesreferences inhandleSelectAllwithselectableChannelIds.
| {selectedChannels.length > 0 && lastSelectedChannel && (() => { | ||
| const channel = channels.find(c => getChannelUniqueId(c) === lastSelectedChannel); | ||
| if (!channel) return null; |
There was a problem hiding this comment.
Settings panel lookup may fail if the channel is no longer in loaded pages.
Line 569 searches only the channels array (derived from infinite-query pages). If the cache is evicted or pages are refetched, the selected channel might not be found, causing the panel to silently disappear. selectedChannelEntities already stores the entity on selection — use it as the primary source, consistent with getSelectedChannels on line 212.
🐛 Suggested fix
{selectedChannels.length > 0 && lastSelectedChannel && (() => {
- const channel = channels.find(c => getChannelUniqueId(c) === lastSelectedChannel);
+ const channel = selectedChannelEntities[lastSelectedChannel] ?? channels.find(c => getChannelUniqueId(c) === lastSelectedChannel);
if (!channel) return null;🤖 Prompt for AI Agents
In `@ui/client/src/features/slack/AddSourceSection.tsx` around lines 568 - 570,
The settings panel lookup should use the stored entity map instead of only
searching the paginated `channels` array; replace the channel lookup in the
block that checks `selectedChannels.length > 0 && lastSelectedChannel`
(currently using `channels.find(c => getChannelUniqueId(c) ===
lastSelectedChannel)`) to first try
`selectedChannelEntities[lastSelectedChannel]` and fall back to
`channels.find(...)` only if the entity map is missing, mirroring the behavior
of `getSelectedChannels` (used around line 212); ensure you reference
`getChannelUniqueId`, `selectedChannelEntities`, `selectedChannels`, and
`lastSelectedChannel` so the panel doesn't disappear when pages are evicted or
refetched.
| const handleSyncChannel = useCallback(async (channel: EmbedChannel) => { | ||
| try { | ||
| await syncSlackChannel(channel.channel_id); | ||
| trackEmbeddingStart([channel.channel_id]); | ||
| refetch(); | ||
| toast({ | ||
| title: "Sync Started", | ||
| description: `Syncing channel "${channel.name}"...`, | ||
| variant: "default", | ||
| }); | ||
| } catch (error) { | ||
| toast({ | ||
| title: "Sync Failed", | ||
| description: `Unable to sync channel "${channel.name}".`, | ||
| variant: "destructive", | ||
| }); | ||
| } | ||
| }, [trackEmbeddingStart, refetch, toast]); |
There was a problem hiding this comment.
AWAITING_SYNC not classified in active/inactive status lists — potential indefinite polling.
After handleSyncChannel succeeds, trackEmbeddingStart adds the channel ID to activeEmbedding, which triggers 5-second polling (line 90-91). However, AWAITING_SYNC is in neither getActiveStatuses() (lines 39-47) nor getInactiveStatuses() (lines 49-54). If the backend keeps the channel in AWAITING_SYNC for an extended period (e.g., waiting for the next scheduled run), the completion-detection useEffect (line 197) will never remove it from activeEmbedding, causing indefinite 5-second polling.
Consider either adding AWAITING_SYNC to getInactiveStatuses (to stop polling and let the scheduled sync handle it) or adding a timeout mechanism to clean stale entries from activeEmbedding.
🤖 Prompt for AI Agents
In `@ui/client/src/features/slack/SlackIntegration.tsx` around lines 178 - 195,
The channel status AWAITING_SYNC isn't classified so channels added by
handleSyncChannel (via trackEmbeddingStart) stay in activeEmbedding and cause
indefinite polling; fix by updating getInactiveStatuses to include
"AWAITING_SYNC" (so status checks stop polling and scheduled runs handle it) or
implement a timeout cleanup that removes stale IDs from activeEmbedding after a
configurable period; update references in getActiveStatuses/getInactiveStatuses,
ensure handleSyncChannel/trackEmbeddingStart and the completion-detection
useEffect correctly respond to the new inactive status or the timeout removal.
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/client/src/features/slack/AddSourceSection.tsx (2)
461-466:⚠️ Potential issue | 🟡 Minor"Showing X of Y" always displays the same value for both X and Y.
filteredChannelsandchannelsare the same array (no client-side filtering, line 236), so this always renders as "Showing N of N loaded channels". The inner count is redundant; consider simplifying to avoid confusing users.🐛 Proposed fix
- <div className="text-sm text-gray-400"> - Showing {filteredChannels.length} of {channels.length} loaded channels - {totalChannels > 0 && channels.length < totalChannels && ( - <span> ({totalChannels} total)</span> - )} - </div> + <div className="text-sm text-gray-400"> + Showing {channels.length} loaded channels + {totalChannels > 0 && channels.length < totalChannels && ( + <span> of {totalChannels} total</span> + )} + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/features/slack/AddSourceSection.tsx` around lines 461 - 466, In AddSourceSection, the JSX currently renders "Showing {filteredChannels.length} of {channels.length} loaded channels" but filteredChannels and channels are identical (no client-side filtering), causing "N of N" confusion; either remove the redundant total (render a single count like "Showing {channels.length} loaded channels") or implement proper client-side filtering so filteredChannels differs from channels; update the render logic in AddSourceSection to use only channels.length (or wire up the filter that produces filteredChannels) and adjust related variables filteredChannels and totalChannels accordingly.
231-233: 🛠️ Refactor suggestion | 🟠 Major
isChannelEmbeddedshould match bychannel_idinstead of display name.The
EmbedChanneltype has achannel_idfield (line 70 ofui/client/src/types/index.ts). The current implementation matches bynameandchannel_name, which are display names that can change or collide (e.g., archived and active channels with the same name). Update the comparison to useembedded.channel_id === channel.channel_idfor reliable matching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/features/slack/AddSourceSection.tsx` around lines 231 - 233, The isChannelEmbedded callback currently matches by display name; update it to match by ID instead—change the comparison inside isChannelEmbedded (used with embedChannels and Channel) from embedded.name === channel.channel_name to embedded.channel_id === channel.channel_id so it reliably uses EmbedChannel.channel_id and Channel.channel_id for matching.
🧹 Nitpick comments (14)
ui/client/src/features/slack/AddSourceSection.tsx (3)
293-303:allNamesis a redundant duplicate ofselectableChannelIds.Both memos produce the same array:
selectableChannelIds(line 293):selectableChannels.map(getChannelUniqueId)allNames(line 298):filteredChannels.filter(!isChannelEmbedded).map(getChannelUniqueId)
selectableChannelsis alreadyfilteredChannels.filter(!isChannelEmbedded). RemoveallNamesand useselectableChannelIdsinhandleSelectAll.♻️ Proposed fix
- const allNames = useMemo(() => - filteredChannels - .filter(c => !isChannelEmbedded(c)) - .map(c => getChannelUniqueId(c)), - [filteredChannels, isChannelEmbedded]); const allSelected = selectableChannelIds.length > 0 && selectableChannelIds.every(id => selectedChannels.includes(id)); const handleSelectAll = useCallback(() => { setSelectedChannels(prev => { if (allSelected) { ... } else { - const newSelection = Array.from(new Set([...prev, ...allNames])); - const newChannels = allNames.filter(id => !prev.includes(id)); + const newSelection = Array.from(new Set([...prev, ...selectableChannelIds])); + const newChannels = selectableChannelIds.filter(id => !prev.includes(id));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/features/slack/AddSourceSection.tsx` around lines 293 - 303, allNames duplicates selectableChannelIds; remove the allNames useMemo and its references and use selectableChannelIds instead (i.e., delete the allNames declaration and update handleSelectAll to iterate/select from selectableChannelIds). Ensure you keep the selectableChannelIds useMemo (which maps selectableChannels via getChannelUniqueId) and remove any imports/usages that solely referenced allNames so no dead identifiers remain.
20-24:SCOPE.ALLandSCOPE.PRIVATEexports and their handler branches are unreachable dead code.
scopeOptionsnow contains onlySCOPE.PUBLIC, meaningsetScopecan never be called withALLorPRIVATE. This leaves the following branches dead:
typesParam:SCOPE.PRIVATEarm (line 123) andSCOPE.ALLfallthrough (line 124)queryFn: thePromise.allblock forSCOPE.ALL(lines 154–177)counts: theSCOPE.ALLandSCOPE.PRIVATEpaths (lines 375–387)If multi-scope support is not planned, removing these simplifies the component considerably. If it may return, at minimum the exports and branches should be documented.
Also applies to: 42-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/features/slack/AddSourceSection.tsx` around lines 20 - 24, SCOPE currently defines ALL and PRIVATE but scopeOptions only exposes PUBLIC, making branches unreachable; either remove the unused SCOPE.ALL and SCOPE.PRIVATE exports and delete the corresponding unreachable branches in typesParam, queryFn (the Promise.all block), and counts, or reintroduce multi-scope support by adding ALL/PRIVATE to scopeOptions and wiring setScope to allow those values; update or add comments near the SCOPE constant and the functions typesParam, queryFn, and counts to document intended behavior if you choose to keep the branches.
477-482:transition-allCSS class conflicts with framer-motion'sanimate-driven width animation;styleis also redundant.Two separate animation systems are controlling
widthsimultaneously:
- The
transition-all duration-500 ease-in-outCSS class will attempt a CSS transition every time thewidthexpression changes.- framer-motion's
animatedrives its own spring transition viasharedTransition.When a motion component is first created, it'll automatically animate to the values in
animateif they're different from those initially rendered, which you can set via CSS or via theinitialprop. Using theinitialprop for the starting state (instead ofstyle) is the idiomatic approach, and the CSStransition-allshould be removed to avoid fighting framer-motion's animation engine.♻️ Proposed fix
<motion.div - className="flex flex-col transition-all duration-500 ease-in-out" - style={{ width: lastSelectedChannel && selectedChannels.length > 0 ? "calc(100% - 420px)" : "100%" }} + className="flex flex-col" + initial={{ width: "100%" }} animate={{ width: lastSelectedChannel && selectedChannels.length > 0 ? "calc(100% - 420px)" : "100%" }} transition={sharedTransition} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/features/slack/AddSourceSection.tsx` around lines 477 - 482, Remove the CSS-driven transition and inline style that conflict with framer-motion: in the motion.div (the element using animate, referencing lastSelectedChannel, selectedChannels and sharedTransition) delete the "transition-all duration-500 ease-in-out" class and remove the style prop; instead set the initial prop to the starting width (use the same expression used for animate: lastSelectedChannel && selectedChannels.length > 0 ? "calc(100% - 420px)" : "100%") and keep animate and transition={sharedTransition} as-is so framer-motion solely controls the width animation.ui/client/src/http/backendClient.ts (1)
13-19: RedundantwithCredentialsoverride in request interceptor.
withCredentials: trueis already set as an instance-level default on Line 10. The request interceptor adds no behaviour and can be removed entirely.♻️ Proposed cleanup
-backendApi.interceptors.request.use( - (config) => { - config.withCredentials = true; - return config; - }, - (error) => Promise.reject(error), -); - backendApi.interceptors.response.use(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/http/backendClient.ts` around lines 13 - 19, The request interceptor registered via backendApi.interceptors.request.use is redundant because the instance-level default already sets config.withCredentials = true; remove the entire interceptor block (the use(...) call that only sets config.withCredentials and rejects errors) so requests rely on the existing default; ensure there are no other side effects expected from backendApi.interceptors.request.use before deleting it and run tests to confirm nothing else depends on that interceptor.backend/admin_config/repository/mongo_repository.py (1)
30-33:create_indexin__init__is a blocking network call.This is acceptable when
MongoAdminConfigRepositoryis a singleton (constructed once at startup), but if MongoDB is unreachable at that moment the entire app container will fail to initialise. This is a deliberate fail-fast pattern, but worth explicitly documenting so future readers understand why a network call lives in the constructor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/admin_config/repository/mongo_repository.py` around lines 30 - 33, The constructor of MongoAdminConfigRepository currently calls self._col.create_index(...) which performs a blocking network call; annotate the __init__ (or the MongoAdminConfigRepository class docstring) with a clear comment explaining this is intentional fail-fast behavior (i.e., the repository is constructed once at startup and should surface connectivity errors immediately), noting the implications if MongoDB is unreachable and alternatives (lazy init or explicit init method) for future maintainers; reference the create_index call in __init__ so readers can quickly find the network call.backend/admin_config/models.py (1)
86-86:field_type: strinFieldValuedrops theLiteralconstraint fromFieldDefinition.
FieldDefinition.field_typeis typed asLiteral["string_list", "string", "boolean", "number"], butFieldValue.field_typewidens it to plainstr. SinceFieldValueis the response model sent to the UI, the narrower type is equally applicable and provides better static guarantees.♻️ Proposed fix
+from typing import Any, Dict, List, Literal, Optional ... class FieldValue(BaseModel): key: str label: str - field_type: str + field_type: Literal["string_list", "string", "boolean", "number"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/admin_config/models.py` at line 86, FieldValue currently widens field_type to plain str; change FieldValue.field_type to the same Literal union used by FieldDefinition (Literal["string_list","string","boolean","number"]) so the response model preserves the narrower type; update the FieldValue dataclass/typing to use that Literal (and add/import typing.Literal if missing) referencing the FieldValue class and FieldDefinition.field_type for consistency.backend/admin_config/action_dispatcher.py (1)
49-62: Address static analysis: narrow the exception type and restructure try/return.Two valid hints from Ruff:
return Trueon Line 59 can move to atry/except/elseblock to clarify intent (TRY300).- Catching bare
Exceptionis overly broad (BLE001) —requests.exceptions.RequestExceptioncovers allrequestsfailure modes.♻️ Suggested fix
url = base_url.rstrip("/") + endpoint try: logger.info("Dispatching action '%s' → %s", action, url) resp = requests.post(url, json={}, timeout=self._timeout) resp.raise_for_status() + except requests.exceptions.RequestException: + logger.exception("Failed to dispatch action '%s' to %s", action, url) + return False + else: logger.info( "Action '%s' dispatched successfully (status=%s)", action, resp.status_code, ) return True - except Exception: - logger.exception("Failed to dispatch action '%s' to %s", action, url) - return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/admin_config/action_dispatcher.py` around lines 49 - 62, The dispatch logic in ActionDispatcher (the block using logger.info, requests.post, resp.raise_for_status and returning True) should use a try/except/else pattern and catch requests.exceptions.RequestException instead of a bare Exception: put the requests.post/resp.raise_for_status and logger.success info inside the try, move the successful return True into an else block, and change the except to except requests.exceptions.RequestException to log via logger.exception("Failed to dispatch action '%s' to %s", action, url) and return False; this narrows the exception handling and clarifies the successful path in the method that calls requests.post and resp.raise_for_status.ui/client/src/features/configuration/StringListField.tsx (1)
20-29: Case-sensitive duplicate check — may allow near-duplicates for usernames.
value.includes(trimmed)is case-sensitive. If this component is used for admin username lists (as the PR suggests),"Admin"and"admin"would both be accepted. Consider whether consumers should pass a case-insensitive comparator or whether the component should normalize case internally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/features/configuration/StringListField.tsx` around lines 20 - 29, The duplicate check in addItem uses value.includes(trimmed) which is case-sensitive and will allow near-duplicates like "Admin" vs "admin"; update addItem to perform a case-insensitive duplicate check (and optionally normalize stored items) by comparing e.g. value.map(v => v.toLowerCase()).includes(trimmed.toLowerCase()) before calling onChange, or add a prop (e.g. comparator or normalizeCase) so consumers can opt into case-insensitive behavior; ensure you still call setInputValue("") and pass the intended normalized or original trimmed value to onChange depending on whether you choose to store normalized usernames.backend/api/flask/endpoints/__init__.py (1)
6-15: Trailing slash in URL prefix whenrouteis empty.When
routeis"", the f-string produces prefixes like/api/health/(note the trailing slash). Depending on how routes are defined in the blueprints, this may cause double-slash URLs (e.g.,/api/health//). Consider conditionally appending the route segment or stripping the trailing slash.♻️ Suggested simplification
for blueprint in backend_blueprints: + parts = f"/api/{blueprint['parent']}" + if blueprint["route"]: + parts += f"/{blueprint['route']}" app.register_blueprint( blueprint["bp"], - url_prefix=f"/api/{blueprint['parent']}/{blueprint['route']}", + url_prefix=parts, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/api/flask/endpoints/__init__.py` around lines 6 - 15, The URL prefix construction can produce a trailing slash when blueprint["route"] is an empty string, causing double-slash paths; update the logic around backend_blueprints/app.register_blueprint so url_prefix omits the extra slash when blueprint["route"] == "" (e.g., compute a prefix variable by conditionally appending f"/{blueprint['route']}" only when blueprint["route"] is truthy or by stripping a trailing slash before calling app.register_blueprint) and use that prefix in the app.register_blueprint call.ui/client/src/features/configuration/AdminConfigSection.tsx (2)
35-38:useEffectdependency on the entiresectionobject reference.The effect resets local edits whenever
sectionchanges by reference. After a save →invalidateQueries→ refetch, a newsectionobject is created, which correctly resets the form. However, if the parent component creates a newsectionreference on every render (e.g., via inline transformation), this could discard in-progress edits unexpectedly. Ensure thesectionprop is referentially stable across parent re-renders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/features/configuration/AdminConfigSection.tsx` around lines 35 - 38, The useEffect currently depends on the whole section object which can change by reference and wipe edits; update the dependency to use stable identifiers instead of the entire object (e.g., use section.id and a stable representation of section.fields or just section.fields if it's an array) so that useEffect only resets when the actual section identity or fields change; locate the useEffect that calls setValues(buildInitialValues(section.fields)) and setIsDirty(false) and replace its dependency array to [section.id, section.fields] (or another stable key/derived stringified fields) to avoid spurious resets caused by new section object references.
139-195:FieldRenderersilently renders nothing for unknownfield_typevalues.If a new field type is added to the template but this component isn't updated, the field will silently disappear from the UI. Consider adding a fallback for unrecognized types.
♻️ Suggested addition after the boolean block
{field.field_type === "boolean" && ( <div className="flex items-center gap-3 mt-1"> <Switch checked={Boolean(value)} onCheckedChange={(checked) => onChange(checked)} /> <span className="text-sm text-gray-400"> {value ? "Enabled" : "Disabled"} </span> </div> )} + + {!["string", "number", "boolean", "string_list"].includes(field.field_type) && ( + <p className="text-xs text-red-400"> + Unsupported field type: {field.field_type} + </p> + )} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/features/configuration/AdminConfigSection.tsx` around lines 139 - 195, FieldRenderer currently returns nothing for unknown field.field_type values; add a visible fallback at the end of the render branch in the FieldRenderer component that handles unrecognized types (e.g., render a warning message/component showing the field.label and field.field_type and optionally a read-only representation of value or field.description) so missing handlers don't silently disappear from the UI; update FieldRenderer to check field.field_type and render this fallback for any case not matched by "string_list", "string", "number", or "boolean" to make unknown types obvious during development and debugging.backend/api/flask/flask_app.py (1)
4-4: Avoidsys.path.appendfor import resolution.Mutating
sys.pathat module level is fragile and can cause hard-to-debug import shadowing. Prefer proper packaging (pyproject.toml/setup.pywithpip install -e .) or settingPYTHONPATHin the dev/deployment environment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/api/flask/flask_app.py` at line 4, The module-level sys.path.append call (sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..')))) should be removed; instead make the project importable via proper packaging or environment configuration: delete that line in flask_app.py, convert the repository into a package (add pyproject.toml/setup.py and appropriate package __init__.py files) and update imports in flask_app.py to use package-relative imports, or set PYTHONPATH / use pip install -e . in your development/deployment environment so the Flask app can resolve imports without mutating sys.path.backend/api/flask/endpoints/admin_config.py (1)
59-66: Unused variable from static analysis.
successis never read. Prefix with_to signal intent.Proposed fix
- success, action = svc.update_section(section_key, values) + _success, action = svc.update_section(section_key, values)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/api/flask/endpoints/admin_config.py` around lines 59 - 66, The local variable "success" returned from svc.update_section(section_key, values) is unused; update the call to either ignore it by prefixing with an underscore (e.g., _success, action = svc.update_section(...)) or remove it and only assign the used value (e.g., _, action = svc.update_section(...)) so static analysis no longer reports an unused variable; locate this in the admin_config_service call site in admin_config.py where current_app.container.admin_config_service and svc.update_section are used and apply the simple rename or destructuring change.ui/client/src/pages/Configuration.tsx (1)
106-108: Silent empty state when categories list is empty.When
config.categories.length === 0, renderingnullgives no feedback to the admin. Consider showing a brief message (e.g., "No configuration categories available") so it's clear the fetch succeeded but there's nothing to display.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/pages/Configuration.tsx` around lines 106 - 108, The component currently returns null when config.categories is empty, producing a silent empty state; change the branch that checks if (!config || config.categories.length === 0) to render a visible empty-state message (for example "No configuration categories available") instead of null so admins know the fetch succeeded but there is nothing to show; update Configuration.tsx to return a small user-facing element (e.g., an EmptyState/NoData component or a simple message wrapper) referencing the same condition and keep existing isLoading/isError handling with TabsSkeleton and ErrorState unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/admin_config/models.py`:
- Line 75: Replace deprecated datetime.utcnow() with timezone-aware
datetime.now(timezone.utc) at each occurrence: update the model field
declaration updated_at: datetime = Field(default_factory=datetime.utcnow) to use
default_factory=lambda: datetime.now(timezone.utc) (or
functools.partial(datetime.now, timezone.utc)) and update the two occurrences in
backend/admin_config/repository/mongo_repository.py (the default/field factories
at the locations using datetime.utcnow on lines referenced) to similarly use
datetime.now(timezone.utc). Also add the required import from datetime: timezone
(e.g., from datetime import datetime, timezone). Ensure all three locations
produce timezone-aware UTC datetimes.
In `@backend/admin_config/repository/mongo_repository.py`:
- Line 48: Replace deprecated datetime.utcnow() usages in mongo_repository.py
with timezone-aware calls: change each "now = datetime.utcnow()" (the two
occurrences assigning to the variable now) to "now =
datetime.now(timezone.utc)". Also update the imports to include timezone (e.g.,
"from datetime import datetime, timezone") if not already imported, and mirror
the same replacement for the other occurrence noted (around the second
assignment to now).
In `@backend/admin_config/service.py`:
- Around line 99-110: Persisting the AdminConfigEntry with self._repo.set can
succeed while self._dispatcher.dispatch later fails, leaving DB updated but
caller getting a 500; change the flow so persistence and dispatch are decoupled:
after calling self._repo.set(entry) and logging the update, wrap the
self._dispatcher.dispatch(...) call in a try/except that catches and logs the
exception (including exception details and
section_key/section_def.on_update_action), and then return success (True,
section_def.on_update_action) to the caller while surfacing the dispatch failure
via a warning log and/or an additional dispatch-failed flag in the response;
alternatively, if rollback is required, attempt to revert the write (e.g.,
remove or restore previous AdminConfigEntry) inside the except before returning
— target symbols: AdminConfigEntry, self._repo.set, self._dispatcher.dispatch,
section_key, section_def.on_update_action.
In `@backend/admin_config/template.py`:
- Line 103: The template currently hardcodes employee SSO usernames in the
default list (the default=["labuyous", "mcarmi", ...] entry) which exposes PII
and is operationally unsafe; replace this hardcoded default by sourcing admin
usernames from runtime configuration instead: remove the literal list from the
template and implement one of the recommended approaches (e.g., read
ADMIN_USERNAMES comma-separated env var at startup and inject it into the
template/DB seed, or set default=[] and provide a bootstrap seed script, or
require the service layer to fetch from DB), ensuring the code that reads the
template (the admin_config/template.py usage of that default) uses the new
runtime-provided value and validates presence before use.
In `@backend/api/flask/endpoints/admin_config.py`:
- Around line 33-35: The current exception handlers in the admin_config Flask
endpoints (get_admin_config, update_section, and access_check) return
jsonify({"error": str(e)}), which can leak internal details; instead, keep the
server-side logger.exception(...) as-is but change the HTTP responses to a
generic error message (e.g., jsonify({"error": "Internal server error"}) or
similar) and a 500 status code so clients don't see str(e); update the return
statements in the except blocks within the functions named get_admin_config,
update_section, and access_check accordingly.
- Around line 93-94: The exception handler currently logs the raw username via
logger.exception("Error checking admin access for '%s'", username), which can
leak PII; change it to log a non-reversible or redacted identifier instead
(e.g., compute a SHA-256 hex or HMAC of username and log a short prefix, or
replace the local-part/domain with a fixed mask) and include the hashed/redacted
value in the logger.exception call within the same except Exception as e block;
ensure any other uses of username in this function (the admin access check) are
similarly replaced so the raw email/username is never written to long-lived
logs.
- Around line 41-46: The update_section endpoint (decorated with
`@admin_config_bp.route`("/config.section.update", methods=["PUT"]) and
implemented by the update_section function) currently allows unauthenticated
callers; add the project’s server-side admin auth decorator (e.g.,
`@admin_required` or `@requires_admin` — import the correct symbol if missing) above
the route decorator so the decorator runs before update_section executes, and
ensure the decorator enforces admin privileges and returns a 403/unauthorized
response for non-admins; keep the existing parameter parsing (section_key,
values) unchanged.
In `@backend/api/flask/flask_app.py`:
- Around line 26-31: The CORS config uses CORS(app, resources={r"/api/*":
{...}}) with origins="*" and supports_credentials=True which is insecure; update
the CORS configuration in flask_app.py to either set origins to your allowed
frontend origin(s) (e.g., read from an env var like FRONTEND_ORIGIN or a list of
trusted URLs) and keep supports_credentials=True only if you need cookies/auth
headers, or keep origins="*" but set supports_credentials=False; modify the
CORS(...) call (the resources dict passed to CORS) to use the chosen origin
values and adjust supports_credentials accordingly so
Access-Control-Allow-Origin is not wildcard when credentials are enabled.
In `@backend/core/app_container.py`:
- Around line 22-43: The MongoClient is only assigned to the local variable
mongo_client inside AppContainer.__init__, so it can't be closed later; change
the init to store the client on the instance (e.g., self.mongo_client =
MongoClient(get_mongo_url())) and use that instance attribute when creating the
DB/collection and MongoAdminConfigRepository (repository=self.admin_config_repo
remains), and ensure callers can call self.mongo_client.close() during shutdown
or tests; update any teardown/shutdown logic to reference self.mongo_client.
In `@backend/entrypoint.sh`:
- Line 13: Add an inline ShellCheck directive immediately above the virtualenv
source to silence SC1090 for the non-constant path: insert a comment like "#
shellcheck source=/dev/null" (or "# shellcheck disable=SC1090" if you prefer)
directly above the ". ~/venv/bin/activate" line in entrypoint.sh so ShellCheck
no longer flags the dynamic activate path.
- Line 14: Update the exec line in entrypoint.sh to quote all env var
substitutions so empty values are passed as empty strings (e.g. use
"$GUNICORN_WORKERS" "$GUNICORN_THREADS" "$PORT" "$GUNICORN_TIMEOUT") and add
simple guards before the exec (or enable set -u) to fail fast with a clear
message if any required vars (GUNICORN_WORKERS, GUNICORN_THREADS, PORT,
GUNICORN_TIMEOUT) are unset/empty; modify the existing exec gunicorn ...
run.wsgi:application invocation to use the quoted expansions and add a small
pre-check that echoes an error and exits when any required variable is missing.
In `@backend/requirements.txt`:
- Around line 1-8: The requirements.txt currently lists unpinned packages
(pydantic, pydantic-settings, gunicorn, flask, flask-cors, pymongo,
python-dotenv, webargs); update backend/requirements.txt to pin each dependency
to specific, tested versions (either exact pins like pydantic==2.11.7 or
constrained ranges like pymongo>=4.15.0,<5.0), then run a freeze to produce a
deterministic lockfile (e.g., requirements.lock via pip freeze) and add that to
the repo/CI; ensure you update the entries for the referenced package names
(pydantic, pymongo, etc.) and commit both the updated requirements.txt and the
generated lockfile.
- Around line 1-8: The project lists Celery as a transitive dependency via
global_utils but has no message broker client, so Celery workers/Beat will fail;
add a broker client (e.g., add the "redis" package or an AMQP client like
"pika") to the backend requirements file where packages pydantic, flask,
pymongo, etc. are declared so Celery Beat and workers can connect to a broker;
update dependency lists used by the backend (or global_utils) and ensure
deployments/CI install the new package.
In `@backend/shared/logger.py`:
- Around line 4-8: The current logging.basicConfig call can be ignored if
handlers already exist; replace it by creating and configuring the module-level
named logger instead: call logger = logging.getLogger(__name__), set
logger.setLevel(logging.INFO), create a logging.StreamHandler(sys.stdout) and
attach a logging.Formatter("%(asctime)s [%(levelname)s] %(name)s: %(message)s")
to that handler, then add the handler to the named logger (and avoid calling
logging.basicConfig or, if you prefer the simpler route, add force=True to the
existing logging.basicConfig(...) call). Ensure you reference
logging.basicConfig, logging.getLogger(__name__), logger.setLevel,
logging.StreamHandler, and logging.Formatter when making the change.
In `@ui/client/src/api/adminConfig.ts`:
- Around line 71-78: The checkAdminAccess function currently sends the username
as a query parameter (backendApi.get('/admin-config/access.check', params: {
username })) which leaks PII into logs and browser history; change the frontend
to POST the username in the request body by replacing backendApi.get call with
backendApi.post('/admin-config/access.check', { username }) and update the
backend endpoint /admin-config/access.check to accept POST with `@from_body`
(replace `@from_query`) so both client function checkAdminAccess and the
corresponding backend handler use a POST body instead of a GET query parameter.
In `@ui/client/src/features/slack/AddSourceSection.tsx`:
- Around line 499-501: The "No channels found matching your search" branch is
unreachable because filteredChannels is set equal to channels (see
filteredChannels assignment) so filteredChannels.length === 0 never occurs;
change the conditional that renders this message to check the debounced search
term instead (e.g., require debouncedSearchTerm to be truthy/ non-empty) while
keeping the existing guards (isLoading, isError) and channels.length > 0; update
the JSX conditional (where filteredChannels, channels, isLoading, isError are
used) to something like: !isLoading && !isError && debouncedSearchTerm &&
channels.length > 0 && filteredChannels.length === 0 so the message appears only
when the user has searched and no results are returned.
In `@ui/client/src/http/backendClient.ts`:
- Line 24: The current console.error call in ui/client/src/http/backendClient.ts
logs the raw Axios error object (console.error('Backend API Error:', error)),
which may leak sensitive request/response data; replace this with a sanitized
log that outputs only safe fields such as error.message plus
error.response?.status and error.config?.url (or other minimal metadata) and
avoid printing the full error object; locate the console.error line in
backendClient.ts and update the logging to extract and log only those safe
properties (and handle non-Axios errors gracefully).
- Around line 21-33: The interceptor in backendApi.interceptors.response.use
currently replaces the original Axios error with new Error(errorMsg), losing
response.status and other metadata; update the handler to preserve the original
error object (or attach status) — e.g., augment the existing error by setting
error.message = errorMsg or error.status = error.response?.status and then
return Promise.reject(error) (or re-throw the original error) instead of
Promise.reject(new Error(errorMsg)); locate this logic around the error
parameter in backendApi.interceptors.response.use and modify it so consumers can
inspect error.response.status while still getting the improved message from
errorMsg.
---
Outside diff comments:
In `@ui/client/src/features/slack/AddSourceSection.tsx`:
- Around line 461-466: In AddSourceSection, the JSX currently renders "Showing
{filteredChannels.length} of {channels.length} loaded channels" but
filteredChannels and channels are identical (no client-side filtering), causing
"N of N" confusion; either remove the redundant total (render a single count
like "Showing {channels.length} loaded channels") or implement proper
client-side filtering so filteredChannels differs from channels; update the
render logic in AddSourceSection to use only channels.length (or wire up the
filter that produces filteredChannels) and adjust related variables
filteredChannels and totalChannels accordingly.
- Around line 231-233: The isChannelEmbedded callback currently matches by
display name; update it to match by ID instead—change the comparison inside
isChannelEmbedded (used with embedChannels and Channel) from embedded.name ===
channel.channel_name to embedded.channel_id === channel.channel_id so it
reliably uses EmbedChannel.channel_id and Channel.channel_id for matching.
---
Duplicate comments:
In `@ui/client/src/features/slack/AddSourceSection.tsx`:
- Around line 567-569: The settings panel currently looks up the selected
channel with channels.find(...) which can return undefined after cache eviction;
change the lookup to first consult the preserved selection map
selectedChannelEntities by key (use selectedChannelEntities[lastSelectedChannel]
or equivalent lookup using getChannelUniqueId) and fall back to
channels.find(...) only if that entry is missing, so the panel still renders
when the cache has been evicted; update the block around lastSelectedChannel in
AddSourceSection.tsx to use selectedChannelEntities as the primary source.
---
Nitpick comments:
In `@backend/admin_config/action_dispatcher.py`:
- Around line 49-62: The dispatch logic in ActionDispatcher (the block using
logger.info, requests.post, resp.raise_for_status and returning True) should use
a try/except/else pattern and catch requests.exceptions.RequestException instead
of a bare Exception: put the requests.post/resp.raise_for_status and
logger.success info inside the try, move the successful return True into an else
block, and change the except to except requests.exceptions.RequestException to
log via logger.exception("Failed to dispatch action '%s' to %s", action, url)
and return False; this narrows the exception handling and clarifies the
successful path in the method that calls requests.post and
resp.raise_for_status.
In `@backend/admin_config/models.py`:
- Line 86: FieldValue currently widens field_type to plain str; change
FieldValue.field_type to the same Literal union used by FieldDefinition
(Literal["string_list","string","boolean","number"]) so the response model
preserves the narrower type; update the FieldValue dataclass/typing to use that
Literal (and add/import typing.Literal if missing) referencing the FieldValue
class and FieldDefinition.field_type for consistency.
In `@backend/admin_config/repository/mongo_repository.py`:
- Around line 30-33: The constructor of MongoAdminConfigRepository currently
calls self._col.create_index(...) which performs a blocking network call;
annotate the __init__ (or the MongoAdminConfigRepository class docstring) with a
clear comment explaining this is intentional fail-fast behavior (i.e., the
repository is constructed once at startup and should surface connectivity errors
immediately), noting the implications if MongoDB is unreachable and alternatives
(lazy init or explicit init method) for future maintainers; reference the
create_index call in __init__ so readers can quickly find the network call.
In `@backend/api/flask/endpoints/__init__.py`:
- Around line 6-15: The URL prefix construction can produce a trailing slash
when blueprint["route"] is an empty string, causing double-slash paths; update
the logic around backend_blueprints/app.register_blueprint so url_prefix omits
the extra slash when blueprint["route"] == "" (e.g., compute a prefix variable
by conditionally appending f"/{blueprint['route']}" only when blueprint["route"]
is truthy or by stripping a trailing slash before calling
app.register_blueprint) and use that prefix in the app.register_blueprint call.
In `@backend/api/flask/endpoints/admin_config.py`:
- Around line 59-66: The local variable "success" returned from
svc.update_section(section_key, values) is unused; update the call to either
ignore it by prefixing with an underscore (e.g., _success, action =
svc.update_section(...)) or remove it and only assign the used value (e.g., _,
action = svc.update_section(...)) so static analysis no longer reports an unused
variable; locate this in the admin_config_service call site in admin_config.py
where current_app.container.admin_config_service and svc.update_section are used
and apply the simple rename or destructuring change.
In `@backend/api/flask/flask_app.py`:
- Line 4: The module-level sys.path.append call
(sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), '..',
'..')))) should be removed; instead make the project importable via proper
packaging or environment configuration: delete that line in flask_app.py,
convert the repository into a package (add pyproject.toml/setup.py and
appropriate package __init__.py files) and update imports in flask_app.py to use
package-relative imports, or set PYTHONPATH / use pip install -e . in your
development/deployment environment so the Flask app can resolve imports without
mutating sys.path.
In `@ui/client/src/features/configuration/AdminConfigSection.tsx`:
- Around line 35-38: The useEffect currently depends on the whole section object
which can change by reference and wipe edits; update the dependency to use
stable identifiers instead of the entire object (e.g., use section.id and a
stable representation of section.fields or just section.fields if it's an array)
so that useEffect only resets when the actual section identity or fields change;
locate the useEffect that calls setValues(buildInitialValues(section.fields))
and setIsDirty(false) and replace its dependency array to [section.id,
section.fields] (or another stable key/derived stringified fields) to avoid
spurious resets caused by new section object references.
- Around line 139-195: FieldRenderer currently returns nothing for unknown
field.field_type values; add a visible fallback at the end of the render branch
in the FieldRenderer component that handles unrecognized types (e.g., render a
warning message/component showing the field.label and field.field_type and
optionally a read-only representation of value or field.description) so missing
handlers don't silently disappear from the UI; update FieldRenderer to check
field.field_type and render this fallback for any case not matched by
"string_list", "string", "number", or "boolean" to make unknown types obvious
during development and debugging.
In `@ui/client/src/features/configuration/StringListField.tsx`:
- Around line 20-29: The duplicate check in addItem uses value.includes(trimmed)
which is case-sensitive and will allow near-duplicates like "Admin" vs "admin";
update addItem to perform a case-insensitive duplicate check (and optionally
normalize stored items) by comparing e.g. value.map(v =>
v.toLowerCase()).includes(trimmed.toLowerCase()) before calling onChange, or add
a prop (e.g. comparator or normalizeCase) so consumers can opt into
case-insensitive behavior; ensure you still call setInputValue("") and pass the
intended normalized or original trimmed value to onChange depending on whether
you choose to store normalized usernames.
In `@ui/client/src/features/slack/AddSourceSection.tsx`:
- Around line 293-303: allNames duplicates selectableChannelIds; remove the
allNames useMemo and its references and use selectableChannelIds instead (i.e.,
delete the allNames declaration and update handleSelectAll to iterate/select
from selectableChannelIds). Ensure you keep the selectableChannelIds useMemo
(which maps selectableChannels via getChannelUniqueId) and remove any
imports/usages that solely referenced allNames so no dead identifiers remain.
- Around line 20-24: SCOPE currently defines ALL and PRIVATE but scopeOptions
only exposes PUBLIC, making branches unreachable; either remove the unused
SCOPE.ALL and SCOPE.PRIVATE exports and delete the corresponding unreachable
branches in typesParam, queryFn (the Promise.all block), and counts, or
reintroduce multi-scope support by adding ALL/PRIVATE to scopeOptions and wiring
setScope to allow those values; update or add comments near the SCOPE constant
and the functions typesParam, queryFn, and counts to document intended behavior
if you choose to keep the branches.
- Around line 477-482: Remove the CSS-driven transition and inline style that
conflict with framer-motion: in the motion.div (the element using animate,
referencing lastSelectedChannel, selectedChannels and sharedTransition) delete
the "transition-all duration-500 ease-in-out" class and remove the style prop;
instead set the initial prop to the starting width (use the same expression used
for animate: lastSelectedChannel && selectedChannels.length > 0 ? "calc(100% -
420px)" : "100%") and keep animate and transition={sharedTransition} as-is so
framer-motion solely controls the width animation.
In `@ui/client/src/http/backendClient.ts`:
- Around line 13-19: The request interceptor registered via
backendApi.interceptors.request.use is redundant because the instance-level
default already sets config.withCredentials = true; remove the entire
interceptor block (the use(...) call that only sets config.withCredentials and
rejects errors) so requests rely on the existing default; ensure there are no
other side effects expected from backendApi.interceptors.request.use before
deleting it and run tests to confirm nothing else depends on that interceptor.
In `@ui/client/src/pages/Configuration.tsx`:
- Around line 106-108: The component currently returns null when
config.categories is empty, producing a silent empty state; change the branch
that checks if (!config || config.categories.length === 0) to render a visible
empty-state message (for example "No configuration categories available")
instead of null so admins know the fetch succeeded but there is nothing to show;
update Configuration.tsx to return a small user-facing element (e.g., an
EmptyState/NoData component or a simple message wrapper) referencing the same
condition and keep existing isLoading/isError handling with TabsSkeleton and
ErrorState unchanged.
| """One persisted config entry keyed by section key.""" | ||
| key: str | ||
| value: Dict[str, Any] = Field(default_factory=dict) | ||
| updated_at: datetime = Field(default_factory=datetime.utcnow) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python 3.12 datetime.utcnow deprecated PEP 615
💡 Result:
datetime.datetime.utcnow() was deprecated in Python 3.12 because it returns a naive datetime (tzinfo is None), and naive datetimes are treated as local time by many datetime operations—making UTC handling error-prone. The docs recommend creating an aware UTC datetime instead. [1]
What to use instead
from datetime import datetime, UTC # Python 3.11+
now_utc = datetime.now(UTC)or:
from datetime import datetime, timezone
now_utc = datetime.now(timezone.utc)Both produce a timezone-aware UTC datetime (recommended). [1]
If you must produce a naive value (e.g., for a legacy API/database that can’t store tzinfo), do it explicitly:
naive_utc = datetime.now(UTC).replace(tzinfo=None)(This makes the “dropping timezone info” step explicit.) [2]
Where PEP 615 fits
PEP 615 is not the reason utcnow() was deprecated. PEP 615 introduced the zoneinfo module (IANA time zones) so you can use real regional time zones (e.g., ZoneInfo("America/New_York")) instead of fixed offsets. [3] The utcnow() deprecation is a separate change focused on avoiding naive datetimes for UTC. [1]
Sources: Python datetime docs (deprecation + replacement) [1], Python core discussion on deprecating utcnow/utcfromtimestamp (rationale and migration patterns) [2], PEP 615 (zoneinfo) [3].
🏁 Script executed:
# Check Python version requirement in the project
find . -maxdepth 2 -type f \( -name "pyproject.toml" -o -name "setup.py" -o -name "requirements.txt" -o -name ".python-version" \) -exec cat {} \;Repository: redhat-community-ai-tools/UnifAI
Length of output: 1865
🏁 Script executed:
# Find and inspect the models.py file mentioned in the review
find . -path "*/backend/admin_config/models.py" -type fRepository: redhat-community-ai-tools/UnifAI
Length of output: 108
🏁 Script executed:
# Find mongo_repository.py files
find . -name "mongo_repository.py" -type fRepository: redhat-community-ai-tools/UnifAI
Length of output: 292
🏁 Script executed:
# Check the models.py file around line 75
head -n 80 ./backend/admin_config/models.py | tail -n 15Repository: redhat-community-ai-tools/UnifAI
Length of output: 730
🏁 Script executed:
# Check imports in models.py
head -n 20 ./backend/admin_config/models.pyRepository: redhat-community-ai-tools/UnifAI
Length of output: 739
🏁 Script executed:
# Check mongo_repository.py files at lines 48 and 67
for file in ./backend/admin_config/repository/mongo_repository.py ./multi-agent/resources/repository/mongo_repository.py ./multi-agent/sharing/repository/mongo_repository.py ./multi-agent/templates/repository/mongo_repository.py; do
echo "=== $file ==="
if [ -f "$file" ]; then
echo "Line 48:"; sed -n '45,50p' "$file"
echo "Line 67:"; sed -n '65,70p' "$file"
fi
doneRepository: redhat-community-ai-tools/UnifAI
Length of output: 2030
🏁 Script executed:
# Check imports in mongo_repository.py
head -n 30 ./backend/admin_config/repository/mongo_repository.pyRepository: redhat-community-ai-tools/UnifAI
Length of output: 889
🏁 Script executed:
# Get the exact lines 48 and 67 with context
sed -n '45,50p' ./backend/admin_config/repository/mongo_repository.py && echo "---" && sed -n '65,70p' ./backend/admin_config/repository/mongo_repository.pyRepository: redhat-community-ai-tools/UnifAI
Length of output: 415
datetime.utcnow() is deprecated in Python 3.12+.
datetime.utcnow() was deprecated in Python 3.12 because it returns a naive datetime object (no timezone info), which can lead to UTC handling errors. The replacement is datetime.now(timezone.utc).
🔧 Proposed fix
-from datetime import datetime
+from datetime import datetime, timezoneThen apply to all three locations:
- updated_at: datetime = Field(default_factory=datetime.utcnow)
+ updated_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc))- now = datetime.utcnow()
+ now = datetime.now(timezone.utc)- updated_at=doc.get("updated_at", datetime.utcnow())
+ updated_at=doc.get("updated_at", datetime.now(timezone.utc))Fix applies to:
backend/admin_config/models.pyline 75backend/admin_config/repository/mongo_repository.pylines 48 and 67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/admin_config/models.py` at line 75, Replace deprecated
datetime.utcnow() with timezone-aware datetime.now(timezone.utc) at each
occurrence: update the model field declaration updated_at: datetime =
Field(default_factory=datetime.utcnow) to use default_factory=lambda:
datetime.now(timezone.utc) (or functools.partial(datetime.now, timezone.utc))
and update the two occurrences in
backend/admin_config/repository/mongo_repository.py (the default/field factories
at the locations using datetime.utcnow on lines referenced) to similarly use
datetime.now(timezone.utc). Also add the required import from datetime: timezone
(e.g., from datetime import datetime, timezone). Ensure all three locations
produce timezone-aware UTC datetimes.
|
|
||
| def set(self, entry: AdminConfigEntry) -> bool: | ||
| """Upsert a config entry.""" | ||
| now = datetime.utcnow() |
There was a problem hiding this comment.
datetime.utcnow() deprecated in Python 3.12+ (same issue as models.py Line 75).
Both occurrences should be replaced with datetime.now(timezone.utc).
🔧 Proposed fix
-from datetime import datetime
+from datetime import datetime, timezone
...
def set(self, entry: AdminConfigEntry) -> bool:
"""Upsert a config entry."""
- now = datetime.utcnow()
+ now = datetime.now(timezone.utc)
...
`@staticmethod`
def _doc_to_entry(doc: dict) -> AdminConfigEntry:
return AdminConfigEntry(
key=doc["key"],
value=doc.get("value", {}),
- updated_at=doc.get("updated_at", datetime.utcnow()),
+ updated_at=doc.get("updated_at", datetime.now(timezone.utc)),
)Also applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/admin_config/repository/mongo_repository.py` at line 48, Replace
deprecated datetime.utcnow() usages in mongo_repository.py with timezone-aware
calls: change each "now = datetime.utcnow()" (the two occurrences assigning to
the variable now) to "now = datetime.now(timezone.utc)". Also update the imports
to include timezone (e.g., "from datetime import datetime, timezone") if not
already imported, and mirror the same replacement for the other occurrence noted
(around the second assignment to now).
| entry = AdminConfigEntry(key=section_key, value=filtered) | ||
| self._repo.set(entry) | ||
|
|
||
| logger.info("Admin config section '%s' updated", section_key) | ||
|
|
||
| self._dispatcher.dispatch( | ||
| action=section_def.on_update_action, | ||
| target=section_def.on_update_target, | ||
| endpoint=section_def.on_update_endpoint, | ||
| ) | ||
|
|
||
| return True, section_def.on_update_action |
There was a problem hiding this comment.
Partial failure: data persists even if dispatch fails.
If self._repo.set(entry) succeeds but self._dispatcher.dispatch(...) throws, the section values are already written to MongoDB while the caller receives a 500 error. This can leave the system in an inconsistent state — the config is updated but the downstream side-effect (e.g., clean_restricted_slack_channels) never fires, and the UI shows a failure so the admin may retry, causing a redundant write.
Consider catching dispatch failures separately and still returning success (with a warning), or wrapping both in a try-block that distinguishes persistence success from dispatch failure.
💡 One possible approach
entry = AdminConfigEntry(key=section_key, value=filtered)
self._repo.set(entry)
logger.info("Admin config section '%s' updated", section_key)
- self._dispatcher.dispatch(
- action=section_def.on_update_action,
- target=section_def.on_update_target,
- endpoint=section_def.on_update_endpoint,
- )
+ try:
+ self._dispatcher.dispatch(
+ action=section_def.on_update_action,
+ target=section_def.on_update_target,
+ endpoint=section_def.on_update_endpoint,
+ )
+ except Exception:
+ logger.exception(
+ "Dispatch failed for section '%s' action '%s'; "
+ "config was saved but side-effect did not execute",
+ section_key, section_def.on_update_action,
+ )
return True, section_def.on_update_action📝 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.
| entry = AdminConfigEntry(key=section_key, value=filtered) | |
| self._repo.set(entry) | |
| logger.info("Admin config section '%s' updated", section_key) | |
| self._dispatcher.dispatch( | |
| action=section_def.on_update_action, | |
| target=section_def.on_update_target, | |
| endpoint=section_def.on_update_endpoint, | |
| ) | |
| return True, section_def.on_update_action | |
| entry = AdminConfigEntry(key=section_key, value=filtered) | |
| self._repo.set(entry) | |
| logger.info("Admin config section '%s' updated", section_key) | |
| try: | |
| self._dispatcher.dispatch( | |
| action=section_def.on_update_action, | |
| target=section_def.on_update_target, | |
| endpoint=section_def.on_update_endpoint, | |
| ) | |
| except Exception: | |
| logger.exception( | |
| "Dispatch failed for section '%s' action '%s'; " | |
| "config was saved but side-effect did not execute", | |
| section_key, section_def.on_update_action, | |
| ) | |
| return True, section_def.on_update_action |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/admin_config/service.py` around lines 99 - 110, Persisting the
AdminConfigEntry with self._repo.set can succeed while self._dispatcher.dispatch
later fails, leaving DB updated but caller getting a 500; change the flow so
persistence and dispatch are decoupled: after calling self._repo.set(entry) and
logging the update, wrap the self._dispatcher.dispatch(...) call in a try/except
that catches and logs the exception (including exception details and
section_key/section_def.on_update_action), and then return success (True,
section_def.on_update_action) to the caller while surfacing the dispatch failure
via a warning log and/or an additional dispatch-failed flag in the response;
alternatively, if rollback is required, attempt to revert the write (e.g.,
remove or restore previous AdminConfigEntry) inside the except before returning
— target symbols: AdminConfigEntry, self._repo.set, self._dispatcher.dispatch,
section_key, section_def.on_update_action.
backend/admin_config/template.py
Outdated
| label="Admin Usernames", | ||
| field_type="string_list", | ||
| description="SSO usernames (preferred_username) with admin access.", | ||
| default=["labuyous", "mcarmi", "nrashti", "oodeh", "osabach", "sfiresht", "yhabushi"], |
There was a problem hiding this comment.
Hardcoded employee SSO usernames constitute PII in version-controlled source code.
The default list on this line embeds real employee identifiers (cross-matched with PR reviewer handles). This is problematic for several reasons:
- Compliance/GDPR — SSO usernames are PII. Once committed they live in git history indefinitely, which conflicts directly with the AIA-Issue-011 compliance goals driving this PR.
- Security — The admin-access list is a security-sensitive artefact; managing it via code requires a deployment to add or remove users.
- Operability — Any team-member change requires a code change, review, and deployment cycle.
Prefer one of:
- Environment variable: read
ADMIN_USERNAMES(comma-separated) at startup and inject into the template or seed the DB. - Empty default + bootstrap script: ship
default=[]and provide a one-time seeding script that runs at first deployment. - Database-only: Remove the
defaultfrom the template entirely; the value is required to be explicitly set before use, enforced by the service layer.
🔧 Example — env-variable approach
+import os
+
+_default_admins = [
+ u.strip()
+ for u in os.environ.get("INITIAL_ADMIN_USERNAMES", "").split(",")
+ if u.strip()
+]
+
FieldDefinition(
key="admin_usernames",
label="Admin Usernames",
field_type="string_list",
description="SSO usernames (preferred_username) with admin access.",
- default=["labuyous", "mcarmi", "nrashti", "oodeh", "osabach", "sfiresht", "yhabushi"],
+ default=_default_admins,
placeholder="e.g. jdoe",
),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/admin_config/template.py` at line 103, The template currently
hardcodes employee SSO usernames in the default list (the default=["labuyous",
"mcarmi", ...] entry) which exposes PII and is operationally unsafe; replace
this hardcoded default by sourcing admin usernames from runtime configuration
instead: remove the literal list from the template and implement one of the
recommended approaches (e.g., read ADMIN_USERNAMES comma-separated env var at
startup and inject it into the template/DB seed, or set default=[] and provide a
bootstrap seed script, or require the service layer to fetch from DB), ensuring
the code that reads the template (the admin_config/template.py usage of that
default) uses the new runtime-provided value and validates presence before use.
| except Exception as e: | ||
| logger.exception("Error getting admin config") | ||
| return jsonify({"error": str(e)}), 500 |
There was a problem hiding this comment.
Avoid leaking internal error details to clients.
str(e) can expose internal implementation details (e.g., connection strings, collection names, stack context) to API consumers. Return a generic message in production and log the real exception server-side (which you already do via logger.exception).
🛡️ Proposed fix
except Exception as e:
logger.exception("Error getting admin config")
- return jsonify({"error": str(e)}), 500
+ return jsonify({"error": "Internal server error"}), 500Apply the same pattern to the update_section and access_check handlers (lines 70-72 and 93-95).
Also applies to: 70-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/api/flask/endpoints/admin_config.py` around lines 33 - 35, The
current exception handlers in the admin_config Flask endpoints
(get_admin_config, update_section, and access_check) return jsonify({"error":
str(e)}), which can leak internal details; instead, keep the server-side
logger.exception(...) as-is but change the HTTP responses to a generic error
message (e.g., jsonify({"error": "Internal server error"}) or similar) and a 500
status code so clients don't see str(e); update the return statements in the
except blocks within the functions named get_admin_config, update_section, and
access_check accordingly.
| logging.basicConfig( | ||
| level=logging.INFO, | ||
| format="%(asctime)s - %(name)s - %(levelname)s - %(message)s" | ||
| format="%(asctime)s [%(levelname)s] %(name)s: %(message)s", | ||
| handlers=[logging.StreamHandler(sys.stdout)], | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python logging.basicConfig force parameter when added Python version
💡 Result:
logging.basicConfig(..., force=...) was added in Python 3.8. It lets basicConfig() remove and close any existing handlers on the root logger before applying the new configuration. [1], [2]
Sources: Python 3.8 “What’s New” (logging section) [1]; Python logging.basicConfig() docs noting “Changed in version 3.8: The force argument was added.” [2]
🏁 Script executed:
# Search for references to "data_pipeline" logger name
rg "data_pipeline" --type py
# Check project Python version requirement
fd -e "setup.py" -o -e "pyproject.toml" -o -e ".python-version" -o -e "requirements.txt" -o -e "setup.cfg" | head -20Repository: redhat-community-ai-tools/UnifAI
Length of output: 585
🏁 Script executed:
# Search for actual getLogger("data_pipeline") calls
rg 'getLogger\s*\(\s*["\']data_pipeline' --type py
# Search for Python version requirement
cat setup.py 2>/dev/null | head -50
cat pyproject.toml 2>/dev/null | head -50
cat .python-version 2>/dev/nullRepository: redhat-community-ai-tools/UnifAI
Length of output: 144
🏁 Script executed:
# Search for getLogger with data_pipeline
rg "getLogger.*data_pipeline" --type py
# Also search more broadly
rg "data_pipeline" --type py -B 2 -A 2
# Check Python version requirement
fd setup.py pyproject.toml .python-version requirements.txt --exec cat {} \;Repository: redhat-community-ai-tools/UnifAI
Length of output: 1755
🏁 Script executed:
# Look at the actual file being reviewed
cat backend/shared/logger.py
# Search for imports of logger from backend/shared
rg "from backend.shared.logger import" --type py
rg "from.*backend.*logger import" --type py
# Check Python version in pyproject.toml or setup.py at root
ls -la | grep -E "setup|pyproject|requirements"
find . -maxdepth 2 -name "pyproject.toml" -o -name "setup.py" -o -name ".python-version"Repository: redhat-community-ai-tools/UnifAI
Length of output: 350
🏁 Script executed:
# Check if backend/shared/logger.py logger is imported elsewhere
rg "from backend.shared" --type py
rg "backend\.shared\.logger" --type py
# Also check for any "platform-backend" references to understand current usage
rg "platform-backend" --type py
# Check the setup.py files to understand Python version
cat ./global_utils/setup.py 2>/dev/null | grep -i python
cat ./rag/setup.py 2>/dev/null | grep -i pythonRepository: redhat-community-ai-tools/UnifAI
Length of output: 1398
🏁 Script executed:
# Check git history to see if backend/shared/logger.py is new or modified
git log --oneline backend/shared/logger.py 2>/dev/null | head -10
# Check if shared-resources/sso-backend/shared/logger.py is a different file
ls -la backend/shared/logger.py shared-resources/sso-backend/shared/logger.py 2>&1
# Check the content of shared-resources version for comparison
cat shared-resources/sso-backend/shared/logger.py 2>/dev/null
# Search for any imports of the logger from backend.shared
rg "import.*logger|from.*logger" backend/ --type py | head -20Repository: redhat-community-ai-tools/UnifAI
Length of output: 593
🏁 Script executed:
# Search specifically for any direct logging.getLogger calls that might reference data_pipeline
rg 'logging\.getLogger\s*\(' backend/ --type py -A 1
# Check what the imports in backend/admin_config look like
cat backend/admin_config/service.py | head -20
cat backend/admin_config/action_dispatcher.py | head -20
# Search for any other references to getLogger in backend directory
rg 'getLogger' backend/ --type pyRepository: redhat-community-ai-tools/UnifAI
Length of output: 1564
logging.basicConfig may be silently ignored — consider force=True or explicit handler on the named logger.
logging.basicConfig is a no-op if the root logger already has handlers (e.g., set up by Flask, Celery, or any imported third-party library before this module is loaded). In that case the stdout handler and format string are never applied and log output silently falls back to whatever was configured first.
Two options:
- Pass
force=True(Python 3.8+, available in this project) to unconditionally reconfigure the root logger:
🔧 Option 1 — add `force=True`
logging.basicConfig(
level=logging.INFO,
format="%(asctime)s [%(levelname)s] %(name)s: %(message)s",
handlers=[logging.StreamHandler(sys.stdout)],
+ force=True,
)- Attach the handler directly to the named logger so it is always applied regardless of root-logger state:
🔧 Option 2 — configure the named logger explicitly
-logging.basicConfig(
- level=logging.INFO,
- format="%(asctime)s [%(levelname)s] %(name)s: %(message)s",
- handlers=[logging.StreamHandler(sys.stdout)],
-)
-
-logger = logging.getLogger("platform-backend")
+logger = logging.getLogger("platform-backend")
+logger.setLevel(logging.INFO)
+if not logger.handlers:
+ _handler = logging.StreamHandler(sys.stdout)
+ _handler.setFormatter(
+ logging.Formatter("%(asctime)s [%(levelname)s] %(name)s: %(message)s")
+ )
+ logger.addHandler(_handler)
+logger.propagate = FalseOption 2 also avoids the side-effect of reconfiguring every other logger in the process (which basicConfig/force=True does), and is safer in test environments where sys.stdout may be replaced after import.
📝 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.
| logging.basicConfig( | |
| level=logging.INFO, | |
| format="%(asctime)s - %(name)s - %(levelname)s - %(message)s" | |
| format="%(asctime)s [%(levelname)s] %(name)s: %(message)s", | |
| handlers=[logging.StreamHandler(sys.stdout)], | |
| ) | |
| logging.basicConfig( | |
| level=logging.INFO, | |
| format="%(asctime)s [%(levelname)s] %(name)s: %(message)s", | |
| handlers=[logging.StreamHandler(sys.stdout)], | |
| force=True, | |
| ) |
| logging.basicConfig( | |
| level=logging.INFO, | |
| format="%(asctime)s - %(name)s - %(levelname)s - %(message)s" | |
| format="%(asctime)s [%(levelname)s] %(name)s: %(message)s", | |
| handlers=[logging.StreamHandler(sys.stdout)], | |
| ) | |
| logger = logging.getLogger("platform-backend") | |
| logger.setLevel(logging.INFO) | |
| if not logger.handlers: | |
| _handler = logging.StreamHandler(sys.stdout) | |
| _handler.setFormatter( | |
| logging.Formatter("%(asctime)s [%(levelname)s] %(name)s: %(message)s") | |
| ) | |
| logger.addHandler(_handler) | |
| logger.propagate = False |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/shared/logger.py` around lines 4 - 8, The current logging.basicConfig
call can be ignored if handlers already exist; replace it by creating and
configuring the module-level named logger instead: call logger =
logging.getLogger(__name__), set logger.setLevel(logging.INFO), create a
logging.StreamHandler(sys.stdout) and attach a logging.Formatter("%(asctime)s
[%(levelname)s] %(name)s: %(message)s") to that handler, then add the handler to
the named logger (and avoid calling logging.basicConfig or, if you prefer the
simpler route, add force=True to the existing logging.basicConfig(...) call).
Ensure you reference logging.basicConfig, logging.getLogger(__name__),
logger.setLevel, logging.StreamHandler, and logging.Formatter when making the
change.
| export async function checkAdminAccess( | ||
| username: string, | ||
| ): Promise<{ is_admin: boolean }> { | ||
| const response = await backendApi.get('/admin-config/access.check', { | ||
| params: { username }, | ||
| }); | ||
| return response.data; | ||
| } |
There was a problem hiding this comment.
Username passed as query parameter leaks PII into logs and browser history.
username (likely an email) is sent as a URL query parameter (?username=alice@example.com). This value will appear in server access logs, CDN/proxy logs, and browser history. Consider switching to a POST with the username in the request body — this also applies to the corresponding backend endpoint (/access.check currently using GET).
Proposed change (frontend side)
export async function checkAdminAccess(
username: string,
): Promise<{ is_admin: boolean }> {
- const response = await backendApi.get('/admin-config/access.check', {
- params: { username },
- });
+ const response = await backendApi.post('/admin-config/access.check', {
+ username,
+ });
return response.data;
}The backend endpoint at backend/api/flask/endpoints/admin_config.py (line 78) would need to change from GET with @from_query to POST with @from_body accordingly.
📝 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 async function checkAdminAccess( | |
| username: string, | |
| ): Promise<{ is_admin: boolean }> { | |
| const response = await backendApi.get('/admin-config/access.check', { | |
| params: { username }, | |
| }); | |
| return response.data; | |
| } | |
| export async function checkAdminAccess( | |
| username: string, | |
| ): Promise<{ is_admin: boolean }> { | |
| const response = await backendApi.post('/admin-config/access.check', { | |
| username, | |
| }); | |
| return response.data; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/client/src/api/adminConfig.ts` around lines 71 - 78, The checkAdminAccess
function currently sends the username as a query parameter
(backendApi.get('/admin-config/access.check', params: { username })) which leaks
PII into logs and browser history; change the frontend to POST the username in
the request body by replacing backendApi.get call with
backendApi.post('/admin-config/access.check', { username }) and update the
backend endpoint /admin-config/access.check to accept POST with `@from_body`
(replace `@from_query`) so both client function checkAdminAccess and the
corresponding backend handler use a POST body instead of a GET query parameter.
| {!isLoading && !isError && filteredChannels.length === 0 && channels.length > 0 && ( | ||
| <div className="p-4 text-center text-gray-500">No channels found matching your search</div> | ||
| )} |
There was a problem hiding this comment.
Empty-search state is unreachable dead code.
Since filteredChannels is assigned directly from channels (line 236) with no filtering, the condition filteredChannels.length === 0 && channels.length > 0 is always false. The "No channels found matching your search" message is never shown.
If client-side filtering was removed intentionally (server-side search now handles it), the check needs to key off debouncedSearchTerm instead:
🐛 Proposed fix
- {!isLoading && !isError && filteredChannels.length === 0 && channels.length > 0 && (
+ {!isLoading && !isError && debouncedSearchTerm && channels.length === 0 && (
<div className="p-4 text-center text-gray-500">No channels found matching your search</div>
)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/client/src/features/slack/AddSourceSection.tsx` around lines 499 - 501,
The "No channels found matching your search" branch is unreachable because
filteredChannels is set equal to channels (see filteredChannels assignment) so
filteredChannels.length === 0 never occurs; change the conditional that renders
this message to check the debounced search term instead (e.g., require
debouncedSearchTerm to be truthy/ non-empty) while keeping the existing guards
(isLoading, isError) and channels.length > 0; update the JSX conditional (where
filteredChannels, channels, isLoading, isError are used) to something like:
!isLoading && !isError && debouncedSearchTerm && channels.length > 0 &&
filteredChannels.length === 0 so the message appears only when the user has
searched and no results are returned.
| backendApi.interceptors.response.use( | ||
| (response) => response, | ||
| (error) => { | ||
| console.error('Backend API Error:', error); | ||
|
|
||
| let errorMsg = 'Failed to fetch data. Please try again.'; | ||
| const errorData = error.response?.data as { error?: string }; | ||
| if (errorData?.error) { | ||
| errorMsg = errorData.error; | ||
| } | ||
|
|
||
| return Promise.reject(new Error(errorMsg)); | ||
| }, |
There was a problem hiding this comment.
Error interceptor swallows HTTP status — callers cannot distinguish auth failures from server errors.
Promise.reject(new Error(errorMsg)) discards the original Axios error, including response.status. Any consumer that needs to handle 401 (redirect to login) or 403 (show "Forbidden" UI) differently from 500 has no way to do so after this interceptor runs.
Consider preserving the status by attaching it to the thrown error, or by re-rejecting the original error after augmenting its message:
🔧 Proposed fix
backendApi.interceptors.response.use(
(response) => response,
(error) => {
console.error('Backend API Error:', error);
let errorMsg = 'Failed to fetch data. Please try again.';
const errorData = error.response?.data as { error?: string };
if (errorData?.error) {
errorMsg = errorData.error;
}
- return Promise.reject(new Error(errorMsg));
+ const err = new Error(errorMsg) as Error & { status?: number };
+ err.status = error.response?.status;
+ return Promise.reject(err);
},
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/client/src/http/backendClient.ts` around lines 21 - 33, The interceptor in
backendApi.interceptors.response.use currently replaces the original Axios error
with new Error(errorMsg), losing response.status and other metadata; update the
handler to preserve the original error object (or attach status) — e.g., augment
the existing error by setting error.message = errorMsg or error.status =
error.response?.status and then return Promise.reject(error) (or re-throw the
original error) instead of Promise.reject(new Error(errorMsg)); locate this
logic around the error parameter in backendApi.interceptors.response.use and
modify it so consumers can inspect error.response.status while still getting the
improved message from errorMsg.
| backendApi.interceptors.response.use( | ||
| (response) => response, | ||
| (error) => { | ||
| console.error('Backend API Error:', error); |
There was a problem hiding this comment.
console.error may log sensitive data from Axios error objects.
The raw Axios error contains the full request config (including cookies/auth headers) and the response body. In a browser devtools console this is readable by anyone with access to the console. Consider logging only error.message or a sanitized subset (e.g., error.response?.status, error.config?.url).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/client/src/http/backendClient.ts` at line 24, The current console.error
call in ui/client/src/http/backendClient.ts logs the raw Axios error object
(console.error('Backend API Error:', error)), which may leak sensitive
request/response data; replace this with a sanitized log that outputs only safe
fields such as error.message plus error.response?.status and error.config?.url
(or other minimal metadata) and avoid printing the full error object; locate the
console.error line in backendClient.ts and update the logging to extract and log
only those safe properties (and handle non-Axios errors gracefully).
1 participant
Restricted Channel Categories
Summary by CodeRabbit
New Services & Components
Backend Service (backend/)
New Features
UI / Behavior
Style