A couple minor fixes #2825
A couple minor fixes #2825
Conversation
Reviewer's GuideThis PR refactors query history insertion to deduplicate via compressed comparison, adds comprehensive CSP reporting headers to the Electron window using new utilities and constants, and augments the AI prompt instructions with additional improvement recommendations. Sequence Diagram for Deduplicated Query History AdditionsequenceDiagram
participant Store
participant QueryEffects_onQuerySuccess as "QueryEffects (onQuerySuccess)";
participant QueryEffects_tryAddHistory as "QueryEffects (tryAddHistory$)";
participant GqlService
Note over Store, QueryEffects_onQuerySuccess: Action SEND_QUERY_REQUEST_ACTION_SUCCESS processed
QueryEffects_onQuerySuccess->>Store: Dispatch historyActions.TryAddHistoryAction(payload)
Store-->>QueryEffects_tryAddHistory: historyActions.TryAddHistoryAction
activate QueryEffects_tryAddHistory
QueryEffects_tryAddHistory->>Store: Get state (history.list)
activate Store
Store-->>QueryEffects_tryAddHistory: Return state
deactivate Store
QueryEffects_tryAddHistory->>GqlService: compress(newQuery from payload)
activate GqlService
GqlService-->>QueryEffects_tryAddHistory: compressedNewQuery
deactivate GqlService
loop For each item in history.list
QueryEffects_tryAddHistory->>GqlService: compress(item.query)
activate GqlService
GqlService-->>QueryEffects_tryAddHistory: compressedItemQuery
deactivate GqlService
end
alt If newQuery (compressed) not in history (compressed)
QueryEffects_tryAddHistory->>Store: Dispatch historyActions.AddHistoryAction(payload)
activate Store
Store-->>Store: Update history state
deactivate Store
end
deactivate QueryEffects_tryAddHistory
Updated Class Diagram for QueryEffectsclassDiagram
class QueryEffects {
+tryAddHistory$: Effect
+constructor(actions$: Actions, store: Store, gqlService: GqlService)
}
class historyActions {
<<Module>>
+TryAddHistoryAction
+AddHistoryAction
}
class GqlService {
+compress(query: string) Promise~string~
}
QueryEffects ..> historyActions : dispatches TryAddHistoryAction
QueryEffects ..> historyActions : handles TryAddHistoryAction
QueryEffects ..> historyActions : dispatches AddHistoryAction
QueryEffects ..> GqlService : uses GqlService.compress()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis update introduces enhancements across several packages. It adds a new AI prompt guideline for suggesting GraphQL query improvements, implements robust query history management using compressed query comparison, and improves security by refactoring Content Security Policy (CSP) handling in the Electron app. New utility functions and constants for Sentry and CSP are also introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Store
participant QueryEffects
participant GqlService
User->>Store: Dispatch TRY_ADD_HISTORY (windowId, query, limit)
Store->>QueryEffects: Effect tryAddHistory$ triggered
QueryEffects->>Store: Get current history for windowId
loop For each history item
QueryEffects->>GqlService: compress(historyItem.query)
QueryEffects->>GqlService: compress(currentQuery)
end
QueryEffects->>QueryEffects: Compare compressed queries
alt No match found
QueryEffects->>Store: Dispatch AddHistoryAction
end
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/altair-api/src/ai/prompt.tsOops! Something went wrong! :( ESLint: 8.18.0 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/packages/altair-api".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "packages/altair-api/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/altair-app/src/app/modules/altair/effects/query.effect.tsOops! Something went wrong! :( ESLint: 8.18.0 ESLint couldn't find the plugin "@angular-eslint/eslint-plugin". (The package "@angular-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/packages/altair-app".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@angular-eslint/eslint-plugin" was referenced from the config file in "packages/altair-app/.eslintrc.js#overrides[0]". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/altair-electron/src/app/window.tsOops! Something went wrong! :( ESLint: 8.18.0 ESLint couldn't find the config "altair" to extend from. Please check that the name of the config is correct. The config "altair" was referenced from the config file in "/packages/altair-electron/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Hello @imolorhe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request titled "A couple minor fixes". This PR addresses two main points: improving the logic for adding queries to the history to avoid duplicates based on compressed query content, and setting up Content Security Policy (CSP) reporting for the Electron application, specifically integrating with Sentry for reporting CSP violations.
Highlights
- Electron CSP Reporting: Content Security Policy reporting has been configured for the Electron application. This adds
report-uriandreport-todirectives to the CSP header, pointing to a Sentry endpoint. This allows the application to automatically report CSP violations, which can help identify potential security issues or misconfigurations in the application's content loading. - AI Prompt Update: A minor addition was made to the instructions provided to the AI assistant, prompting it to suggest recommendations for improving the security, performance, or usability of GraphQL queries when appropriate.
Changelog
Click here to see the changelog
- packages/altair-api/src/ai/prompt.ts
- Added a new instruction to the AI prompt to suggest security, performance, or usability recommendations for GraphQL queries (line 30).
- packages/altair-app/src/app/modules/altair/effects/query.effect.ts
- Replaced the direct history addition logic with a dispatch of the new
TryAddHistoryAction(lines 209-216). - Added a new effect
tryAddHistory$that listens forTRY_ADD_HISTORYactions, compresses the query and existing history items, and dispatchesAddHistoryActiononly if the compressed query is not found in history (lines 1432-1475).
- Replaced the direct history addition logic with a dispatch of the new
- packages/altair-app/src/app/modules/altair/store/history/history.action.ts
- Defined a new action type
TRY_ADD_HISTORY(line 5). - Defined the
TryAddHistoryActionclass (lines 22-26). - Added
TryAddHistoryActionto theActiontype union (line 28).
- Defined a new action type
- packages/altair-electron/src/app/window.ts
- Imported
cspAsStringandSENTRY_CSP_REPORT_URI(lines 44-45). - Removed
https://apis.google.comfrom thescriptSrclist in the CSP header (line 309). - Added
Content-Security-Policy,Report-To, andReporting-Endpointsheaders to the response headers, configuring CSP reporting to Sentry (lines 315-342). - Used the new
cspAsStringutility to format the CSP header value (lines 318-323).
- Imported
- packages/altair-electron/src/constants.ts
- Created a new file to define constants for Sentry DSN and CSP report URI (lines 1-3).
- packages/altair-electron/src/index.ts
- Imported
SENTRY_DSNfrom the new constants file (line 6). - Used the
SENTRY_DSNconstant for Sentry initialization (line 9).
- Imported
- packages/altair-electron/src/utils/csp.spec.ts
- Added a new file containing a unit test for the
cspAsStringutility function (lines 1-22).
- Added a new file containing a unit test for the
- packages/altair-electron/src/utils/csp.ts
- Created a new file containing the
cspAsStringutility function to format CSP directives into a string, handling quoted values and prefixes (lines 1-26).
- Created a new file containing the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Small fixes arrive,
History learns compressed truth,
Security reports.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a couple of useful fixes: refining the logic for adding queries to history by comparing compressed versions, and enhancing security by adding a CSP report URI for the Electron app. The new utility for generating CSP strings and its tests are well-implemented.
I've identified a few areas for potential improvement, primarily concerning efficiency and error handling in the history management effect, and a clarification needed for a CSP change. I'm defaulting to common TypeScript best practices for this review as no specific style guide was provided.
Summary of Findings
- Efficiency in History Query Compression: In
packages/altair-app/src/app/modules/altair/effects/query.effect.ts, the new query is compressed multiple times within a loop when checking against history items. Compressing it once outside the loop would be more efficient. - Error Handling in Async Operations: In
packages/altair-app/src/app/modules/altair/effects/query.effect.ts, potential promise rejections fromgqlService.compress()within the history check logic are not explicitly handled, which could lead to unhandled rejections. - CSP
script-srcModification: Inpackages/altair-electron/src/app/window.ts,https://apis.google.comwas removed from thescript-srcCSP directive. This needs confirmation that it's intentional and won't break existing functionality. - Existing TODO in CSP Configuration: The file
packages/altair-electron/src/app/window.ts(line 316) contains aTODO: Figure out why an error from this breaks devtoolsrelated to CSP. While this PR modifies CSP, this specific TODO might be pre-existing. It's worth keeping in mind as CSP settings are adjusted. (Not commented inline due to severity settings).
Merge Readiness
The pull request makes some good improvements. However, there are a few points that would benefit from further attention, particularly the potential efficiency issue in query history management and the clarification needed for the CSP script-src change. Addressing the high and medium severity comments would improve the robustness and performance of these changes. I am unable to approve this pull request myself; please ensure these points are considered and have other reviewers approve the code before merging.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/altair-electron/src/utils/csp.ts (1)
11-26: Robust CSP string formatting implementation.The
cspAsStringfunction correctly implements CSP directive formatting according to the specification. The logic properly handles:
- Quoting special values ('self', 'unsafe-inline', etc.)
- Quoting hash and nonce values
- Proper directive and value joining
Consider adding JSDoc documentation:
+/** + * Converts a CSP object to a properly formatted CSP string. + * @param csp - Object where keys are directive names and values are arrays of sources + * @returns Formatted CSP string ready for use in headers + */ export const cspAsString = (csp: Record<string, string[]>): string => {packages/altair-app/src/app/modules/altair/effects/query.effect.ts (1)
1451-1456: Consider performance optimization for large history lists.Compressing all history queries on each check could impact performance with large history lists. Consider caching compressed queries or implementing a more efficient deduplication strategy.
For better performance with large history lists, consider storing pre-compressed queries:
- const matchesPromise = res.data.history.list.map(async (item) => { - return ( - (await this.gqlService.compress(item.query)) === - (await this.gqlService.compress(res.action.payload.query)) - ); - }); + const compressedQuery = await this.gqlService.compress(res.action.payload.query); + const matchesPromise = res.data.history.list.map(async (item) => { + // Consider storing compressed queries in history items to avoid repeated compression + const compressedHistoryQuery = await this.gqlService.compress(item.query); + return compressedHistoryQuery === compressedQuery; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/altair-api/src/ai/prompt.ts(1 hunks)packages/altair-app/src/app/modules/altair/effects/query.effect.ts(2 hunks)packages/altair-app/src/app/modules/altair/store/history/history.action.ts(2 hunks)packages/altair-electron/src/app/window.ts(2 hunks)packages/altair-electron/src/constants.ts(1 hunks)packages/altair-electron/src/index.ts(1 hunks)packages/altair-electron/src/utils/csp.spec.ts(1 hunks)packages/altair-electron/src/utils/csp.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: tests / test (macos-latest)
- GitHub Check: tests / e2e-test (ubuntu-latest)
- GitHub Check: electron / electron (macos-latest)
- GitHub Check: tests / api-e2e-test
- GitHub Check: tests / e2e-test (macos-latest)
- GitHub Check: electron / electron (windows-latest)
- GitHub Check: tests / test (ubuntu-latest)
- GitHub Check: electron / electron (ubuntu-latest)
- GitHub Check: sites / Deploy sites
- GitHub Check: docker / docker
- GitHub Check: build_and_preview
🔇 Additional comments (7)
packages/altair-electron/src/constants.ts (1)
1-3:❌ Incorrect review comment
Security concern: Hardcoded sensitive API keys should be externalized
These constants contain Sentry API keys that should not be hardcoded in the source code. Exposing these keys in a public repository could lead to unauthorized usage and potential security issues.
Consider moving these to environment variables:
-export const SENTRY_DSN = - 'https://1b08762f991476e3115e1ab7d12e6682@o4506180788879360.ingest.sentry.io/4506198594813952'; -export const SENTRY_CSP_REPORT_URI = `https://o4506180788879360.ingest.us.sentry.io/api/4506198594813952/security/?sentry_key=1b08762f991476e3115e1ab7d12e6682`; +export const SENTRY_DSN = process.env.SENTRY_DSN || ''; +export const SENTRY_CSP_REPORT_URI = process.env.SENTRY_CSP_REPORT_URI || '';Also ensure these environment variables are:
- Added to
.env.examplewith placeholder values- Documented in the README
- Properly configured in your CI/CD pipeline
🌐 Web query:
Are Sentry DSN keys safe to expose in client-side applications?💡 Result:
Sentry DSN (Data Source Name) keys are not considered sensitive secrets, but they should still be treated with some caution.
Explanation:
- The Sentry DSN is publicly included in your client-side application code (e.g., JavaScript running in the browser) so that errors can be reported directly to your Sentry project.
- It does not grant full access to your Sentry project or account. Specifically, it cannot be used to view data or make changes.
- However, if someone obtains your DSN, they could send fake error reports to your project, potentially cluttering your error logs.
- To mitigate this, Sentry allows you to restrict the origins (domains) from which events can be sent.
- For server-side secrets like API keys or tokens with full project access, you should never expose them in client-side apps.
In summary:
- It is safe to embed the Sentry DSN in client-side applications with proper domain restrictions set on the Sentry side.
- Do not treat it as a sensitive secret, but do monitor for abuse or misuse.
Would you like guidance on how to set domain restrictions in Sentry or best practices for securing your Sentry integration?
No action required for security of Sentry DSN
The Sentry DSN is designed to be public in client-side code and does not grant access beyond reporting events. Hardcoding it here does not pose a secret-leak risk. To prevent abuse (e.g. spam events), configure domain restrictions in your Sentry project settings:
• Go to Project Settings → Security & Privacy → Allowed Domains
• Add your application’s domains/originsIf you prefer per-environment configuration (e.g. dev vs prod), you can still source the DSN from build-time environment variables, but this is for convenience and not for security.
Likely an incorrect or invalid review comment.
packages/altair-electron/src/app/window.ts (1)
315-337: Well-implemented CSP reporting configuration!The CSP headers are properly configured with:
- Dynamic CSP generation using the
cspAsStringutility- Both
report-uri(legacy) andreport-to(modern) directives for compatibility- Proper JSON formatting for the Report-To header
- Reporting-Endpoints header for the Reporting API v1
packages/altair-electron/src/index.ts (1)
6-6:✅ Verification successful
Good refactoring to centralize configuration.
Extracting the Sentry DSN to a constants file improves maintainability and follows configuration management best practices.
Let me verify that the constants file properly exports the SENTRY_DSN:
Also applies to: 9-9
🏁 Script executed:
#!/bin/bash # Verify that the constants file exists and exports SENTRY_DSN cat packages/altair-electron/src/constants.ts | grep -E "(SENTRY_DSN|export)"Length of output: 266
I want to confirm that
SENTRY_DSNis actually assigned a value (and not left empty). Let’s inspect theconstants.tsfile contents:
🏁 Script executed:
#!/bin/bash # Show the first 200 lines of constants.ts to verify SENTRY_DSN assignment sed -n '1,200p' packages/altair-electron/src/constants.tsLength of output: 342
Refactoring Approved: SENTRY_DSN Properly Centralized
The
constants.tsfile exports a non-emptySENTRY_DSNvalue, and the import inpackages/altair-electron/src/index.tsis valid. No further changes required.packages/altair-electron/src/utils/csp.ts (1)
1-10: Well-defined CSP constants following specification.The constants correctly identify CSP values and prefixes that require single quotes according to the CSP specification.
packages/altair-api/src/ai/prompt.ts (1)
30-30: Valuable addition to AI prompt guidelines.The new recommendation to suggest improvements for security, performance, or usability is a valuable enhancement that encourages the AI to provide more comprehensive and helpful responses.
packages/altair-app/src/app/modules/altair/store/history/history.action.ts (1)
5-5:✅ Verification successful
Well-implemented NgRx action following established patterns.
The new
TryAddHistoryActionfollows the established NgRx action pattern with proper typing and structure. The payload design is consistent and the union type update ensures type safety.Let me verify that this action is properly handled in the effects:
Also applies to: 22-26, 28-28
🏁 Script executed:
#!/bin/bash # Verify that TRY_ADD_HISTORY is handled in effects rg -A 10 "TRY_ADD_HISTORY" packages/altair-app/src/app/modules/altair/effects/Length of output: 1177
TryAddHistoryAction properly handled in effects
The new
TryAddHistoryActionfollows established NgRx patterns with correct typing and is correctly consumed in
packages/altair-app/src/app/modules/altair/effects/query.effect.tsviaofType(historyActions.TRY_ADD_HISTORY).No further changes needed—approving.
packages/altair-app/src/app/modules/altair/effects/query.effect.ts (1)
209-217: Good architectural improvement: Separated history management logic.Moving the history management to a dedicated effect via
TryAddHistoryActionimproves code organization and follows NgRx best practices for separation of concerns.
| 'sha256-abc123', | ||
| 'https://example.com', | ||
| "'nonce-xyz789'", | ||
| 'unsafe-eval', |
There was a problem hiding this comment.
Fix CSP keyword formatting: 'unsafe-eval' should be quoted
The CSP keyword unsafe-eval must be enclosed in single quotes according to the CSP specification.
- 'unsafe-eval',
+ "'unsafe-eval'",This test would currently fail because the expected string on line 19 shows 'unsafe-eval' with quotes, but the input on line 12 is missing them.
Also applies to: 19-19
🤖 Prompt for AI Agents
In packages/altair-electron/src/utils/csp.spec.ts at line 12, the string
'unsafe-eval' is missing quotes, which causes the test to fail since the
expected value on line 19 includes quotes. Add single quotes around unsafe-eval
in the string at line 12 to match the CSP specification and ensure the test
passes.
| ], | ||
| 'Report-To': JSON.stringify({ | ||
| group: 'csp-endpoint', | ||
| max_age: 10886400, // 3 months |
There was a problem hiding this comment.
Fix incorrect comment: max_age value doesn't match 3 months
The comment states "3 months" but the value 10886400 seconds equals approximately 126 days (4.2 months), not 3 months.
For exactly 3 months (90 days), use:
- max_age: 10886400, // 3 months
+ max_age: 7776000, // 3 months (90 days)Or update the comment to match the current value:
- max_age: 10886400, // 3 months
+ max_age: 10886400, // ~4 months (126 days)📝 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.
| max_age: 10886400, // 3 months | |
| max_age: 10886400, // ~4 months (126 days) |
🤖 Prompt for AI Agents
In packages/altair-electron/src/app/window.ts at line 327, the comment
indicating "3 months" for the max_age value is incorrect because 10886400
seconds is approximately 4.2 months. To fix this, either update the comment to
reflect the actual duration represented by the value or change the value to
match exactly 3 months (90 days). Ensure the comment and value are consistent
for clarity.
| tryAddHistory$ = createEffect( | ||
| () => { | ||
| return this.actions$.pipe( | ||
| ofType(historyActions.TRY_ADD_HISTORY), | ||
| withLatestFrom( | ||
| this.store, | ||
| (action: historyActions.TryAddHistoryAction, state: RootState) => { | ||
| return { | ||
| data: state.windows[action.payload.windowId], | ||
| windowId: action.payload.windowId, | ||
| action, | ||
| }; | ||
| } | ||
| ), | ||
| switchMap((res) => { | ||
| if (!res.data) { | ||
| return EMPTY; | ||
| } | ||
|
|
||
| const matchesPromise = res.data.history.list.map(async (item) => { | ||
| return ( | ||
| (await this.gqlService.compress(item.query)) === | ||
| (await this.gqlService.compress(res.action.payload.query)) | ||
| ); | ||
| }); | ||
|
|
||
| return from(Promise.all(matchesPromise)).pipe( | ||
| map((matches) => { | ||
| if (!matches.includes(true)) { | ||
| // If the query is not in history, add it | ||
| this.store.dispatch( | ||
| new historyActions.AddHistoryAction(res.windowId, { | ||
| query: res.action.payload.query, | ||
| limit: res.action.payload.limit, | ||
| }) | ||
| ); | ||
| } | ||
| }) | ||
| ); | ||
| }) | ||
| ); | ||
| }, | ||
| { dispatch: false } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for compression operations.
The effect lacks error handling for potential compression failures. If gqlService.compress throws an error, the entire effect will fail.
Wrap the compression logic in error handling:
switchMap((res) => {
if (!res.data) {
return EMPTY;
}
- const matchesPromise = res.data.history.list.map(async (item) => {
- return (
- (await this.gqlService.compress(item.query)) ===
- (await this.gqlService.compress(res.action.payload.query))
- );
- });
-
- return from(Promise.all(matchesPromise)).pipe(
+ return from(
+ this.gqlService.compress(res.action.payload.query).catch((err) => {
+ debug.error('Failed to compress query for history check', err);
+ return null;
+ })
+ ).pipe(
+ switchMap((compressedQuery) => {
+ if (!compressedQuery) {
+ // Skip history check if compression failed
+ return EMPTY;
+ }
+
+ const matchesPromise = res.data!.history.list.map(async (item) => {
+ try {
+ const compressedHistoryQuery = await this.gqlService.compress(item.query);
+ return compressedHistoryQuery === compressedQuery;
+ } catch (err) {
+ debug.error('Failed to compress history query', err);
+ return false;
+ }
+ });
+
+ return from(Promise.all(matchesPromise));
+ }),
map((matches) => {
if (!matches.includes(true)) {📝 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.
| tryAddHistory$ = createEffect( | |
| () => { | |
| return this.actions$.pipe( | |
| ofType(historyActions.TRY_ADD_HISTORY), | |
| withLatestFrom( | |
| this.store, | |
| (action: historyActions.TryAddHistoryAction, state: RootState) => { | |
| return { | |
| data: state.windows[action.payload.windowId], | |
| windowId: action.payload.windowId, | |
| action, | |
| }; | |
| } | |
| ), | |
| switchMap((res) => { | |
| if (!res.data) { | |
| return EMPTY; | |
| } | |
| const matchesPromise = res.data.history.list.map(async (item) => { | |
| return ( | |
| (await this.gqlService.compress(item.query)) === | |
| (await this.gqlService.compress(res.action.payload.query)) | |
| ); | |
| }); | |
| return from(Promise.all(matchesPromise)).pipe( | |
| map((matches) => { | |
| if (!matches.includes(true)) { | |
| // If the query is not in history, add it | |
| this.store.dispatch( | |
| new historyActions.AddHistoryAction(res.windowId, { | |
| query: res.action.payload.query, | |
| limit: res.action.payload.limit, | |
| }) | |
| ); | |
| } | |
| }) | |
| ); | |
| }) | |
| ); | |
| }, | |
| { dispatch: false } | |
| ); | |
| tryAddHistory$ = createEffect( | |
| () => { | |
| return this.actions$.pipe( | |
| ofType(historyActions.TRY_ADD_HISTORY), | |
| withLatestFrom( | |
| this.store, | |
| (action: historyActions.TryAddHistoryAction, state: RootState) => { | |
| return { | |
| data: state.windows[action.payload.windowId], | |
| windowId: action.payload.windowId, | |
| action, | |
| }; | |
| } | |
| ), | |
| switchMap((res) => { | |
| if (!res.data) { | |
| return EMPTY; | |
| } | |
| return from( | |
| this.gqlService.compress(res.action.payload.query).catch((err) => { | |
| debug.error('Failed to compress query for history check', err); | |
| return null; | |
| }) | |
| ).pipe( | |
| switchMap((compressedQuery) => { | |
| if (!compressedQuery) { | |
| // Skip history check if compression failed | |
| return EMPTY; | |
| } | |
| const matchesPromise = res.data!.history.list.map(async (item) => { | |
| try { | |
| const compressedHistoryQuery = await this.gqlService.compress(item.query); | |
| return compressedHistoryQuery === compressedQuery; | |
| } catch (err) { | |
| debug.error('Failed to compress history query', err); | |
| return false; | |
| } | |
| }); | |
| return from(Promise.all(matchesPromise)); | |
| }), | |
| map((matches) => { | |
| if (!matches.includes(true)) { | |
| // If the query is not in history, add it | |
| this.store.dispatch( | |
| new historyActions.AddHistoryAction(res.windowId, { | |
| query: res.action.payload.query, | |
| limit: res.action.payload.limit, | |
| }) | |
| ); | |
| } | |
| }) | |
| ); | |
| }) | |
| ); | |
| }, | |
| { dispatch: false } | |
| ); |
🤖 Prompt for AI Agents
In packages/altair-app/src/app/modules/altair/effects/query.effect.ts lines 1432
to 1475, the compression operations using gqlService.compress are not wrapped
with error handling, which could cause the effect to fail if an error occurs
during compression. Wrap each call to gqlService.compress with try-catch blocks
or use Promise.catch to handle errors gracefully, ensuring that errors are
logged or managed without disrupting the effect flow. This will improve
robustness and prevent unhandled promise rejections during compression failures.
|
Visit the preview URL for this PR (updated for commit bb0e96c): https://altair-gql--pr2825-imolorhe-minor-updat-y3lpfe32.web.app (expires Mon, 02 Jun 2025 14:43:01 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 02d6323d75a99e532a38922862e269d63351a6cf |
There was a problem hiding this comment.
Hey @imolorhe - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
General comments:
- In the TryAddHistoryEffect you’re manually dispatching AddHistoryAction inside a map with dispatch:false—consider returning the AddHistoryAction from the stream and letting NgRx dispatch it to follow recommended effect patterns and improve traceability.
- Compressing every history item on each TryAddHistoryAction can become a performance bottleneck for large histories; consider caching compressed results or limiting comparisons to keep the UI responsive.
- Hard-coding SENTRY_DSN and report URIs in the source exposes secrets and hinders multi-env configuration—move them to environment variables or secure config files instead.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return EMPTY; | ||
| } | ||
|
|
||
| const matchesPromise = res.data.history.list.map(async (item) => { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the query compression and deduplication logic into a helper service to keep the effect focused on RxJS wiring.
| const matchesPromise = res.data.history.list.map(async (item) => { | |
| // Extract the “compress + dedupe” logic into a small helper so the effect is just wiring RxJS operators. | |
| // e.g. in history.service.ts | |
| export class HistoryService { | |
| constructor(private gql: GqlService) {} | |
| async shouldAdd(query: string, history: { query: string }[]): Promise<boolean> { | |
| const compressedNew = await this.gql.compress(query); | |
| for (const entry of history) { | |
| if (await this.gql.compress(entry.query) === compressedNew) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } | |
| } | |
| // Then simplify the effect: | |
| tryAddHistory$ = createEffect(() => | |
| this.actions$.pipe( | |
| ofType(historyActions.TryAddHistoryAction), | |
| withLatestFrom( | |
| this.store.select(s => s.windows), // adjust selector to get windows map | |
| (action, windows) => ({ action, history: windows[action.payload.windowId]?.history.list }) | |
| ), | |
| filter(({ history }) => !!history), | |
| concatMap(({ action, history }) => | |
| from(this.historyService.shouldAdd(action.payload.query, history!)).pipe( | |
| filter(should => should), | |
| map(() => | |
| new historyActions.AddHistoryAction(action.payload.windowId, { | |
| query: action.payload.query, | |
| limit: action.payload.limit | |
| }) | |
| ) | |
| ) | |
| ) | |
| ) | |
| ); |
Benefits:
- Single async call per history entry – no more
Promise.all/ nestedmap(async…). - Clear separation of concerns – effect just composes operators, dedupe logic lives in one service.
- Easier to test – you can unit‐test
HistoryService.shouldAddin isolation.
| @@ -0,0 +1,3 @@ | |||
| export const SENTRY_DSN = | |||
| 'https://1b08762f991476e3115e1ab7d12e6682@o4506180788879360.ingest.sentry.io/4506198594813952'; | |||
| export const SENTRY_CSP_REPORT_URI = `https://o4506180788879360.ingest.us.sentry.io/api/4506198594813952/security/?sentry_key=1b08762f991476e3115e1ab7d12e6682`; | |||
There was a problem hiding this comment.
security (generic-api-key): Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
Source: gitleaks
Fixes
Checks
yarn test-buildChanges proposed in this pull request:
Summary by Sourcery
Improve query history handling, enhance Electron CSP reporting, add CSP utility and constants, update AI prompt guidelines, and cover CSP utility with tests.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Refactor
Tests